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

Add support for "rewritten urls" in module detection #1095

Closed
wants to merge 4 commits into from

Conversation

@dweeves
Copy link

@dweeves dweeves commented Apr 10, 2019

This is a fix for issue #1023 & duplicated #1093

The code adds a new method to get module information from url.
This method is first used on the actual symfony request pathinfo
if no valid module is found, then the same method is applied on the real 'PATH_INFO' (which will be the rewritten on) .

The last commit is now supporting urls without trailiing slash (/register)

Copy link
Member

@tvdijen tvdijen left a comment

I hope you don't mind, but I took care of some coding style..
We've recently put a lot of effort in this, so I'm a bit keen on it..

Besides that, this is a neat PR!
I'd like a review from the person who introduced Symfony routing before I'm willing to merge this though!

Loading

@tvdijen tvdijen requested a review from jaimeperez Apr 10, 2019
@dweeves
Copy link
Author

@dweeves dweeves commented Jul 24, 2019

Any news on final approval ?

Loading

@tvdijen
Copy link
Member

@tvdijen tvdijen commented Jul 24, 2019

Hi @dweeves !

I've asked Jaime again to take a look at this PR. I know there are several version of 'routing improvement' lying around and we need to pick a strategy here. A finalized version of the routing-feature will be part of a v1.18-release, hopefully somewhere in Q4 this year depending on resources.

Loading

@tvdijen tvdijen added this to the 1.18 milestone Jul 24, 2019
Copy link
Member

@jaimeperez jaimeperez left a comment

Thanks a lot for your contribution, @dweeves!

If I understand the issue correctly, it actually lies in Symfony, and this is just a workaround. Shouldn't we try to get this fixed upstream?

Essentially, introducing an abstraction layer (offloading request parsing to Symfony, instead of using $_SERVER directly) serves a purpose, that being making it easier to test all code and increase coverage as much as possible. If we are still checking for $_SERVER['PATH_INFO'], we'll still have problems to test that code.

Note though that I'm just thinking out loud here 😅

Loading

// the path must always be on the form /module/
throw new Error\NotFound('The URL must at least contain a module name followed by a slash.');
if ($basepathinfo === '/') {
throw new Error\NotFound('No PATH_INFO to module.php');
Copy link
Member

@jaimeperez jaimeperez Jul 25, 2019

Choose a reason for hiding this comment

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

This error would be shown to the end user, but won't help much, I'm afraid. Should we have an error message similar to the old one?

Loading

Copy link
Author

@dweeves dweeves Aug 22, 2019

Choose a reason for hiding this comment

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

I agree that the error message is not giving much valuable information, so using the old one is a good option.

About the use of $_SERVER['path_info'] , i didn't find a way to have it through Symfony request. i had a look at the code inside symfony and the thing is they are trying to be "too smart" about request parsing, providing a "too high" abstraction level above the initial request. so loosing information with no way to get it back from the original request. So the only way i found was to ask the original request details directly through $_SERVER.

Loading

Copy link
Member

@jaimeperez jaimeperez Sep 4, 2019

Choose a reason for hiding this comment

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

Uhm, I see...

Do you think this is actually wrong behaviour in Symfony? Should we report it upstream for them to consider a change (regardless of whether we work around it or not)?

Loading

Copy link
Author

@dweeves dweeves Sep 5, 2019

Choose a reason for hiding this comment

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

I think this is a behaviour that is not ideal for the purpose we want to achieve (ie, detecting which module is targeted by the rewritten url) , i cannot say it's a "bad" behaviour from symfony, it's just a choice that was made that is making the request object not well suited for low level url access but very interesting to process request content & structure.
Maybe adding a new method to the request object to get the "target" url should be a nice addition. however i suppose the code of this method would only have to return $_SERVER['PATH_INFO'] ;)

Loading

@tvdijen tvdijen removed this from the 1.18 milestone Aug 29, 2019
@tvdijen tvdijen added this to the 1.19 milestone Aug 29, 2019
@jaimeperez
Copy link
Member

@jaimeperez jaimeperez commented Sep 12, 2019

Hi @dweeves!

I think I've found a way to circumvent Symfony's parsing of the context such that it works for our case, without need to be messing around with $_SERVER. I created a patch and it's available here. Would you mind applying it and telling me if it works for you too?

Thanks in advance!

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants