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

Avoid booting providers because they require a Silex\Application instance #72

Merged
merged 2 commits into from
Sep 2, 2015

Conversation

Seldaek
Copy link
Contributor

@Seldaek Seldaek commented Sep 1, 2015

@lsmith77
Copy link
Member

lsmith77 commented Sep 1, 2015

looks like some requirements do not work for php 5.3/5.4 which is causing the build failures.

as for the change can you explain why they shoild not be booted?

@Seldaek
Copy link
Contributor Author

Seldaek commented Sep 1, 2015

The build failure is independent from my PR really, it's just that Silex bumped the requirement to php 5.5.9 so we should drop the 5.3/5.4 builds here IMO for 2.0

As for why not booting them, as discussed in the linked issue the boot() method requires a Silex\Application, which Cilex\Application is not, so skipping the boot makes everything work fine it just means we might miss some features that are set up in the boot methods but it is seldom used and surely better than a fatal error.

@stof
Copy link
Member

stof commented Sep 1, 2015

btw, the order of methods was weird before that. Providers were supposed to subscribe before booting. It was inconsistent with Silex.

@stof
Copy link
Member

stof commented Sep 1, 2015

The use statement should be removed too

@Seldaek
Copy link
Contributor Author

Seldaek commented Sep 2, 2015

OK I removed the use statement and also did a second commit for dependencies/travis update, it's kinda unrelated but while I'm at it it brings this package fully up to speed with Silex 2.0-dev.

lsmith77 added a commit that referenced this pull request Sep 2, 2015
Avoid booting providers because they require a Silex\Application instance
@lsmith77 lsmith77 merged commit ad8162a into Cilex:develop Sep 2, 2015
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