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 for new way of handling scheme in Symfony 3.0 #254

Merged
merged 1 commit into from
Aug 9, 2017
Merged

Fix for new way of handling scheme in Symfony 3.0 #254

merged 1 commit into from
Aug 9, 2017

Conversation

tobias-93
Copy link
Collaborator

Since Symfony 2.2, the way to go to provide the scheme (and method) for a routing entry is no longer via the requirements: key but through the schemes key.
Although this has worked throughout the 2.x versioning of Symfony, in 3.x the deprecated way has been removed.
This commit fixes the handling to comply to the new way of supplying the scheme to Symfony, while preserving (and favouring) the old behaviour.

@rrajkomar
Copy link

rrajkomar commented Nov 10, 2016

Hi,

@willdurand : When could this MR be integrated in the project ?
Symfony is already at its 3.2 version (master) and this is still not implemented, forcing people to use a deprecated version for scheme requirements...
Thanks.

@rrajkomar
Copy link

🆙

@tobias-93
Copy link
Collaborator Author

@stof @willdurand Could you please look at this PR? It is one of the only blocking dependencies for our project to migrate to Symfony 3.
Thank you!

@apsylone
Copy link

apsylone commented Feb 7, 2017

@stof @willdurand +1 Please merge...

@tobias-93
Copy link
Collaborator Author

Apparently, I did not fix all of the broken functionality. I just fixed that (included schemes in the externs.js). Now this fix is absolutely ready to be merged.

@tobias-93
Copy link
Collaborator Author

@stof @willdurand Anything I can do to get this merged?

@spiroski
Copy link

Is this project maintained at all?

@tobias-93
Copy link
Collaborator Author

@stof Sorry to bother you but I'd really appreciate if you would take a look at this.

@tobias-93
Copy link
Collaborator Author

@stof C'mon, this is getting embarrassing. What is wrong with this PR?

Copy link
Member

@GuilhemN GuilhemN left a comment

Choose a reason for hiding this comment

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

LGTM though I don't use this bundle so my opinion on this has not much value :)

@@ -56,6 +56,8 @@ public function getRoutes()
'defaults' => $defaults,
'requirements' => $route->getRequirements(),
'hosttokens' => method_exists($compiledRoute, 'getHostTokens') ? $compiledRoute->getHostTokens() : array(),
'methods' => $route->getMethods(),
Copy link
Member

Choose a reason for hiding this comment

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

Is this used somewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@GuilhemN it isn't, but as this could be defined in the requirements, similar to the schemes, before Symfony 3, I migrated that as well so someone that uses it could make it work. I could remove this or add it to the prototype in the JS file as well. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

it isn't, but as this could be defined in the requirements, similar to the schemes, before Symfony 3, I migrated that as well so someone that uses it could make it work.

Ok, good point 👍

I could remove this or add it to the prototype in the JS file as well.

Imo you should add it to the JS prototype :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, done 😄

@GuilhemN GuilhemN merged commit 8e01507 into FriendsOfSymfony:master Aug 9, 2017
@GuilhemN
Copy link
Member

GuilhemN commented Aug 9, 2017

Thank you @tobias-93!

@tobias-93
Copy link
Collaborator Author

@GuilhemN Thank you so much for merging! Would it be possible to cherry-pick this commit to branch 1.x (if not, I will gladly make a new PR) and tag a new release? Thanks!

@GuilhemN
Copy link
Member

GuilhemN commented Aug 9, 2017

You're welcome :)
That'd be easier if you could make a PR (I didn't clone the repo). I will then release a patch.

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.

5 participants