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

Made PHPCR initializer independent, creating all route_basepaths #318

Merged
merged 2 commits into from
Dec 17, 2015

Conversation

pamil
Copy link
Contributor

@pamil pamil commented Dec 3, 2015

Related PR Sylius/Sylius#3668.

@pamil pamil changed the title Route basepaths initializer Made PHPCR initializer independent, creating all route_basepaths Dec 3, 2015
@pamil
Copy link
Contributor Author

pamil commented Dec 3, 2015

Failed only on PHP7, may be connected with enabled XDebug on Travis.

@@ -25,6 +25,7 @@
"matthiasnoback/symfony-dependency-injection-test": "~0.6",
"matthiasnoback/symfony-config-test": "0.*",
"phpunit/php-code-coverage": "@stable",
"phpunit/phpunit": "^4.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please do not depend on phpunit

@pamil pamil force-pushed the route-basepaths-initializer branch from 115453e to bb0e113 Compare December 3, 2015 12:43
@dbu
Copy link
Member

dbu commented Dec 3, 2015

thanks!
i restarted the builds that failed. as master is not failing, i think it should work. if it still does not, we can try restarting the build for master to see if with the final release of php 7, there was some change that makes us break.

@pamil pamil force-pushed the route-basepaths-initializer branch from 75fc700 to 656aef1 Compare December 3, 2015 13:23
@pamil
Copy link
Contributor Author

pamil commented Dec 3, 2015

I disabled XDebug and it passes :)

@pjedrzejewski
Copy link

👍

@wouterj
Copy link
Member

wouterj commented Dec 3, 2015

So there is now a bC break in this PR: While route basepaths were not initialized by default before, they are now. I think we the RoutingBundle should keep using the old configuration and not initialize the basepaths unless configured.

@dbu
Copy link
Member

dbu commented Dec 3, 2015 via email

@lsmith77 lsmith77 added review and removed wip/poc labels Dec 7, 2015
@dbu
Copy link
Member

dbu commented Dec 14, 2015

@pamil can you adjust the PR to use auto for the initializer?

@pamil
Copy link
Contributor Author

pamil commented Dec 14, 2015

@dbu Sure, thanks for reminding me this PR, I almost forgot about it. I'm on my way! :)

@pamil
Copy link
Contributor Author

pamil commented Dec 14, 2015

@dbu should be up and running now.

@pamil
Copy link
Contributor Author

pamil commented Dec 17, 2015

Only StyleCI is failing and it's (mostly) not related to my changes.

@dantleech
Copy link
Member

@pamil you should be able to rebase: try (on your branch):

$ git fetch origin
$ git rebase origin/master

If it encounters any conflicts you will need to fix them and then:

$ git add /fixed/files
$ git rebase --continue

If you have any difficulties let us know

@pamil pamil force-pushed the route-basepaths-initializer branch 2 times, most recently from db7db13 to 3f8bc5d Compare December 17, 2015 14:28
@pamil
Copy link
Contributor Author

pamil commented Dec 17, 2015

@dantleech thanks, I must have missed something, now the rebase went all easy :) Style CI is passing, tests should be passing.

@dbu
Copy link
Member

dbu commented Dec 17, 2015

great, this looks good now!
can you please squash the commits into 1 commit plus 1 for the travis config change? (git rebase -i... then reorder the commits and squash the last two into the first one)

…ing all defined route_basepaths if enable_initializer=true
@pamil pamil force-pushed the route-basepaths-initializer branch from 3f8bc5d to 49e6cb1 Compare December 17, 2015 15:07
@pamil
Copy link
Contributor Author

pamil commented Dec 17, 2015

@dbu done :)

@dbu
Copy link
Member

dbu commented Dec 17, 2015

thanks. one last thing: can you please do a pull request on the symfony-cmf-docs repository to update the configuration references of the routing bundle? i think its enough to mention the feature there. but if you want you can look over the routing documentation and see if there is a place where it would make sense to mention this feature. please use the versionadded functionality (this feature will be in version 1.4)

dbu added a commit that referenced this pull request Dec 17, 2015
Made PHPCR initializer independent, creating all route_basepaths
@dbu dbu merged commit 9641d8b into symfony-cmf:master Dec 17, 2015
@lsmith77 lsmith77 removed the review label Dec 17, 2015
@dbu
Copy link
Member

dbu commented Dec 17, 2015

thanks a lot. created symfony-cmf/symfony-cmf-docs#724 for the documentation.

@pjedrzejewski
Copy link

👍

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.

None yet

6 participants