Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace FileType with ImageType for images #65

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kochen
Copy link

@kochen kochen commented Oct 17, 2017

fixes #31
replaces #63

@michalmarcinkowski
Copy link
Member

Thanks @kochen! Could you have a look at the failing build?

@kochen
Copy link
Author

kochen commented Oct 17, 2017

Could not find a defined element with name "block". The defined ones are: cmf-block.

@michalmarcinkowski could it be that:

$this->getElement('block')

needs to actually be:

$this->getElement('cmf-block')

and it has nothing to do with my fix?

@kochen
Copy link
Author

kochen commented Oct 17, 2017

@michalmarcinkowski the fix I proposed, solves issue #31, allowing existing blocks which already contain image to be edited and image to be added/replaced.

But, when a new (i.e custom) block is created with a new image, there is an error:
The node '/cms/media' already has a child named 'image''.
which is eventually what causes the test to fail.

Any idea how to solve this one?

@michalmarcinkowski
Copy link
Member

How is it possible that the tests pass on master branch, but not on yours? Have you tried to debug which of your change cased the error?

@kochen
Copy link
Author

kochen commented Oct 18, 2017

@michalmarcinkowski because I replaced FileType with ImageType for images and it seem to behave differently.

@kochen
Copy link
Author

kochen commented Oct 30, 2017

@michalmarcinkowski simply because there are no test for "editing a block with image" :/

@michalmarcinkowski
Copy link
Member

michalmarcinkowski commented Oct 31, 2017

That's bad... @kochen are you able to fix the issue?

@kochen
Copy link
Author

kochen commented Oct 31, 2017

@michalmarcinkowski not from the little time I invested in it.
The problem seems to be much deeper than this plugin and CmfMediaBundle is abandoned anyway so you should consider some alternatives for the future anyway...

@michalmarcinkowski
Copy link
Member

Thanks for the feedback, if you will have any more information that could help, please share it with us 😉

@kochen
Copy link
Author

kochen commented Oct 31, 2017

@michalmarcinkowski first thing I'm trying to do is write some tests which will show the problem.
Any help you could provide here (specially on the factory/image)?

@kochen
Copy link
Author

kochen commented Oct 31, 2017

@michalmarcinkowski the problem is somewhere in the actual generation of the image:
image

instead of belonging to the actual content-block parent, it is independent and therefor a second block with an image will try to save another /cms/media/image which already exists...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The form's view data is expected File
2 participants