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

4.x router cleanup #13670

Merged
merged 3 commits into from Sep 23, 2019
Merged

4.x router cleanup #13670

merged 3 commits into from Sep 23, 2019

Conversation

ADmad
Copy link
Member

@ADmad ADmad commented Sep 23, 2019

  • Fix extra _params key being set in $context passed to Route::match() call on line 484.
  • Optimize Router::fullBaseUrl() to return early when called as getter with Router::$_fullBaseUrl already set.

With this change setting App.fullBaseUrl after Router::fullBaseUrl()
is called will have no effect.
@ADmad ADmad added this to the 4.0.0 milestone Sep 23, 2019
@ADmad
Copy link
Member Author

ADmad commented Sep 23, 2019

There's currently one inconsistent behaviour regarding full URL generation which should be addressed.

Normally when generating full URL Router::url() takes the host from $_requestContext but when _ssl key is set it get's the host by parsing the URL returned by Router::fullBaseUrl(). Normally this is not an issue as the host is usually same for both but it would create discrepancy if App.fullBaseUrl is set with a host which is not the same as current host got from current request URI.

Also calling parse_url() everytime _ssl key is set seems quite wasteful. Shouldn't we just update Router::$_requestContext['_host'] when Router::fullBaseUrl() is called? I believe the host available from App.fullBaseUrl should always take precedence.

@markstory
Copy link
Member

Also calling parse_url() everytime _ssl key is set seems quite wasteful. Shouldn't we just update Router::$_requestContext['_host'] when Router::fullBaseUrl() is called? I believe the host available from App.fullBaseUrl should always take precedence.

Do you mean parsing the URL the first time the full base url is read from configure? We could store that in the _requestContext and use that instead.

@markstory markstory merged commit e01e681 into 4.x Sep 23, 2019
@markstory markstory deleted the 4.x-router-cleanup branch September 23, 2019 14:51
@ADmad
Copy link
Member Author

ADmad commented Sep 23, 2019

Do you mean parsing the URL the first time the full base url is read from configure? We could store that in the _requestContext and use that instead.

Yup, that's what I meant.

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

2 participants