[WIP] Refactoring to support 2.0 #214
Conversation
|
Looking good. I guess you are also addressing #213 with the |
| 2.0.0-alpha1 | ||
| ------------ | ||
|
|
||
| * **2014-10-30**: PhpcrMenuProvider now requires the NodeLoader as first argument instead of a FactoryInterface |
There was a problem hiding this comment.
i would explicitly say we updated to knp menu 2.0 and then list the BC breaks caused by that. and it seems to me there are more than this line.
|
looks quite good, seems we can clean up some stuff but its not a huge impact for users of the bundle in the end. i guess one problematic thing could be other bundles using knp menu but restricting to 1.x. just checked, at least SonataAdminBundle supports both 1.1 and 2 we need to update the documentation, for example about the voters (link to knp doc instead of explaining cmf_menu.voter, but keep explaining our cmf voters) |
Yeah, that's something I'll find out when updating the sandbox, SE and website. :) At least, I needed to do this since the kunstmaan bundles only support KnpMenu 2.0. |
|
what is missing here? |
|
@dbu some failing PRs, but mostly trouble with the sandbox and missing features in KnpMenu 2.0. I'm going to open a PR there soon and hope they'll merge it quick (and release a new minor version). |
|
great. anything we can help with at this point? |
|
I don't think so. See KnpLabs/KnpMenu#189 for the Knp PR. |
|
needs a rebase |
Using inheritance here would mean too much copy/pasting. The current way of doing this is not compatible with Sf <2.5, there should properbly be some compatibility tag in the CoreBundle for this.
Refs commit: KnpLabs/KnpMenu@5056422 Again, this allows us to have less duplication.
|
Any help needed? |
|
No, this PR is almost ready. However, we're still waiting for some KnpMenu contributor to merge KnpLabs/KnpMenuBundle#251 |
|
This PR is now finally passing. The bundle itself is ready, but I haven't used it in a more complex application like the sandbox, because of missing features in the KnpMenuBundle 2.0 (for instance, determining if a menu item was current in a template). This will be changed in 2.1, but there are no release plans yet. This wonders which direction we should take: Leave this in 2.0-dev/beta till KnpMenuBundle 2.1 is released and release a 1.3 release together with CMF 1.3 or release a 2.0 version of this bundle, knowing that it would be a bit harder to use it in projects. |
|
doh .. the wait on KnpMenu never seems to stop :( |
|
i think releasing a cmf menu 2.0 that has severe regressions is not a good idea, so rather lets wait - or see if we can speed up things for knp menu 2.1 ourselves? can you link all issues that are a problem for us, so that we know what to look at / when things would be ready? |
|
@dbu I've created an issue in the KnpMenu repository to ask the maintainers about the status. I've pinged you about it. I hope they (maybe with help of us) will be able to wrap things up soon. SonataAdminBundle 2.4 now requires KnpMenuBundle 2, so we have to merge this in order to use the new Sonata bundles. |
[WIP] Refactoring to support 2.0
|
can you open follow up tickets for anything that still needs to be done? |
|
will do |
While updating to 2.0, I also tried to make the code cleaner and easier to understand. Since this will be released as CmfMenuBundle 2.0, we can have BC breaks in it. This is kind of the last change to make things really better.
Todo