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

Fix Imagine Adapter due to last ImagineBundle commits #106

Merged
merged 1 commit into from
Jul 31, 2014

Conversation

JJK801
Copy link
Contributor

@JJK801 JJK801 commented May 20, 2014

Hi guys,

I had some troubles to implement an ImagineBlock, this is due to last commits on LiipImagineBundle which change some namespaces.

This PR fix it.

Thanks

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? no
Fixed tickets ~
License MIT
Doc PR ~
  • fix the tests as they have not been updated yet
  • submit changes to the documentation

@lsmith77
Copy link
Member

hmm we should have a conflict in place to prevent you from combining with current master:
https://github.com/symfony-cmf/MediaBundle/blob/master/composer.json#L26

the issue is that LiipImagineBundle currently is too much of a moving target for us to try and stay compatible ..

@JJK801
Copy link
Contributor Author

JJK801 commented May 20, 2014

My first approach was to set imagine bundle to an older version, but it didn't solved as the routing file was added in the last alpha:

None of the chained routers were able to generate route: Route _imagine_home_slideshow not found

If you want reproduce my troubles, try to implements a SlideshowBlock using Symfony CMF (http://symfony.com/doc/current/cmf/bundles/block/types.html#create-your-first-slideshow)

@dbu
Copy link
Member

dbu commented May 22, 2014

hi @JJK801 i just checked and in the sandbox i managed to create an ImagineBlock and upload an image into it. i ran composer update on the sandbox two weeks ago, so at least back then it resolved to something usable. did you trick composer into installing a later LiipImagineBundle?

i think we eventually need to update the CmfMediaBundle to use the LiipImagineBundle and if somebody can track their progress and validate what they do, for example how liip/LiipImagineBundle#219 (comment) affects us, that would be great.

for the 1.1 version, i think we need to fix on something that we know will work with the version of LiipImagineBundle < 1.0

@JJK801
Copy link
Contributor Author

JJK801 commented May 26, 2014

hi @dbu,

I switched all my symfony-cmf bundles on the dev-master version (i did the with LiipImagineBundle) in order to improve it, as it's not a profesional project (associative) i can let it work as is and send some PR when i get troubles.

Actually, the last troubles i get are the one fixed by this PR and another with JQueryBundle for SonataAdmin.

for the 1.1 version, i think we need to fix on something that we know will work with the version of LiipImagineBundle < 1.0

IMO, it will be a good idea to extract imagine functionalities into a separated bundle (which will follow the LiipImagineBundle versions), to improve maintainability.

Thanks,

Jérémy

@lsmith77
Copy link
Member

1.0 has been released https://github.com/liip/LiipImagineBundle/releases/tag/1.0.0
i think we should fix MediaBundle 1.1 to work with both <1.0 and also >=1.0

@dbu
Copy link
Member

dbu commented May 30, 2014

If that is doable then yes.

For 1.2 i would remove all pre 1.0 imagine bundle things from mediabundle. Maybe we can start with that and then backport imagine 1.0 support into Media 1.1 if doable.

@JJK801
Copy link
Contributor Author

JJK801 commented May 30, 2014

👍

@dbu
Copy link
Member

dbu commented Jun 4, 2014

the tests are currently failing. @JJK801 can you work on the upgrade and remove workarounds for pre 1.0 imagine bundle things? we also would need a doc update i guess. best see http://symfony.com/doc/current/contributing/code/patches.html#make-a-pull-request and use this template in the PR header so that we don't forget anything.

there are a couple of things to investigate, for example liip/LiipImagineBundle#219 (comment)

@dbu
Copy link
Member

dbu commented Jun 4, 2014

another issue to sync with is liip/LiipImagineBundle#375 (comment)

@JJK801
Copy link
Contributor Author

JJK801 commented Jun 4, 2014

@dbu no problem, i'm a little bit busy by work for now but i will fix/improve it all asap (probably next week)

@dbu
Copy link
Member

dbu commented Jun 12, 2014

the mediabundle master is now aliased to 1.2 so once the changes are done, we can merge and start to depend on imagine bundle 1.0.* only.

@JJK801
Copy link
Contributor Author

JJK801 commented Jul 29, 2014

@JJK801
Copy link
Contributor Author

JJK801 commented Jul 30, 2014

Coverage fail ⁉️ 😿

@dbu
Copy link
Member

dbu commented Jul 30, 2014

we have seen that in a couple of bundles now. php 5.3 is EOL. lets add it to the allowed failures in travis.yml

@dbu
Copy link
Member

dbu commented Jul 30, 2014

this looks good!

there are open boxes in the todo list at the top, can you check those? is there anything else missing? did you check if symfony-cmf-docs mentions anything of the liip imaginebundle version?

@JJK801
Copy link
Contributor Author

JJK801 commented Jul 30, 2014

@dbu i'm on it, it will be good in few minutes

@JJK801
Copy link
Contributor Author

JJK801 commented Jul 30, 2014

@dbu that seems all good for me :)

@dbu
Copy link
Member

dbu commented Jul 30, 2014

awesome. can you please squash the commits and edit the .travis.yml to allow failures with php 5.3?

@JJK801
Copy link
Contributor Author

JJK801 commented Jul 30, 2014

BTW, excuse me for the PR duration time, i was very busy...

@JJK801
Copy link
Contributor Author

JJK801 commented Jul 30, 2014

@dbu done

dbu added a commit that referenced this pull request Jul 31, 2014
Fix Imagine Adapter due to last ImagineBundle commits
@dbu dbu merged commit f7d50c9 into symfony-cmf:master Jul 31, 2014
@dbu
Copy link
Member

dbu commented Jul 31, 2014

awesome, thanks a lot!

@JJK801
Copy link
Contributor Author

JJK801 commented Jul 31, 2014

my pleasure :)

@JJK801 JJK801 deleted the imagine-fix branch July 31, 2014 07:02
wouterj added a commit to symfony-cmf/symfony-cmf-docs that referenced this pull request Oct 21, 2014
This PR was submitted for the master branch but it was merged into the dev branch instead (closes #531).

Discussion
----------

Add imagine routing to the configuration

Hi,

This PR contains documentation changes related to symfony-cmf/media-bundle#106

Thanks,

Jérémy

Commits
-------

34311c1 Add imagine routing to the configuration
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.

3 participants