Skip to content

[HttpFoundation] Fix for virtualhosts based on URL path #38614

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 15, 2020
Merged

[HttpFoundation] Fix for virtualhosts based on URL path #38614

merged 1 commit into from
Nov 15, 2020

Conversation

mvorisek
Copy link
Contributor

Q A
Branch? 3.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #34866
License MIT
Doc PR no

This PR fixes base URL detection when:

  • virtualhost is based on URL path
  • AND local path does not match that URL virtual host path prefix

fix covered with tests

@mvorisek mvorisek force-pushed the fix_path_discovery_root_in_subdir branch 4 times, most recently from debd6b6 to 02b65f3 Compare October 17, 2020 19:28
@mvorisek mvorisek marked this pull request as draft October 17, 2020 19:32
@derrabus derrabus added this to the 3.4 milestone Oct 17, 2020
@mvorisek mvorisek marked this pull request as ready for review October 17, 2020 19:46
@mvorisek mvorisek changed the title Fix #34866 - virtualhost is based on URL path Fix #34866 - virtualhost based on URL path Oct 17, 2020
@mvorisek mvorisek force-pushed the fix_path_discovery_root_in_subdir branch from 10a3fb3 to 2515016 Compare October 17, 2020 19:48
@nicolas-grekas nicolas-grekas changed the title Fix #34866 - virtualhost based on URL path [HttpFoundation] Fix for virtualhosts based on URL path Oct 19, 2020
@nicolas-grekas nicolas-grekas force-pushed the fix_path_discovery_root_in_subdir branch from 2515016 to 7ebf3c8 Compare October 19, 2020 14:38
@nicolas-grekas nicolas-grekas force-pushed the fix_path_discovery_root_in_subdir branch from 7ebf3c8 to 8cf2489 Compare October 19, 2020 14:50
@fabpot fabpot modified the milestones: 3.4, 4.4 Oct 28, 2020
@fabpot
Copy link
Member

fabpot commented Nov 3, 2020

This one should be rebased on 4.4 before merging.

@nicolas-grekas nicolas-grekas changed the base branch from 3.4 to 4.4 November 3, 2020 11:58
@nicolas-grekas nicolas-grekas force-pushed the fix_path_discovery_root_in_subdir branch from 8cf2489 to 75ff868 Compare November 3, 2020 11:58
@nicolas-grekas
Copy link
Member

(now rebased for 4.4)

@derrabus
Copy link
Member

Thank you @mvorisek.

@derrabus derrabus merged commit 091265b into symfony:4.4 Nov 15, 2020
@mvorisek mvorisek deleted the fix_path_discovery_root_in_subdir branch November 15, 2020 22:44
@apfelbox
Copy link
Contributor

@mvorisek does this also fix #37995?

@mvorisek
Copy link
Contributor Author

mvorisek commented Nov 16, 2020 •
edited
Loading

@mvorisek does this also fix #37995?

Hi Anna, not sure about this one, but I think my previous PR #34868 will probably fix it. Feel free to pursue that PR that code.

@fabpot fabpot mentioned this pull request Nov 21, 2020
Merged
This was referenced Nov 29, 2020
Merged
Merged
@BafS
Copy link
Contributor

BafS commented Jan 18, 2021 •
edited
Loading

This PR is a BC for us, I thought it was related to Symfony 5.2 upgrade but after digging, it was introduced in patch version, Symfony 5.1.9.

If you rewrite URL like domain.com/index.php/api/... => api.domain.com/... it will work but if the URI contains api it will break.

For example if you have api.domain.com/foo/api/bar then the first if line 1851 will be true and $basename will be changed from index.php to api which makes bugs when matching routes.

@nicolas-grekas
Copy link
Member

Thanks for the notice @BafS
Did you find a workaround?
Can you post it and this comment on a new issue?
As you may know, comments on closed PRs are not tracked.

@BafS BafS mentioned this pull request Jan 18, 2021
Closed
BafS added a commit to BafS/symfony that referenced this pull request Jan 18, 2021
@BafS
Revert symfony#38614, add assert to avoid regression
bb774d2
@BafS BafS mentioned this pull request Jan 18, 2021
Merged
BafS added a commit to BafS/symfony that referenced this pull request Jan 18, 2021
@BafS
Revert symfony#38614, add assert to avoid regression
3058cd0
fabpot added a commit that referenced this pull request Jan 18, 2021
@fabpot
bug #39886 [HttpFoundation] Revert #38614 and add assert to avoid reg…
6dd5078 
…ressions (BafS)

This PR was merged into the 4.4 branch.

Discussion
----------

[HttpFoundation] Revert #38614 and add assert to avoid regressions

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #39882
| License       | MIT

#38614 introduced a BC, this PR revert the PR, update tests and add an assert to avoid regressions.

Commits
-------

3058cd0 Revert #38614, add assert to avoid regression
derrabus added a commit that referenced this pull request Jan 19, 2021
@derrabus
Merge branch '4.4' into 5.1
c639531 
* 4.4:
  [HttpFoundation] Drop int return type from parseFilesize()
  Added $translator->addLoader()
  bug #39878 [doctrine-bridge] Add username to UserNameNotFoundException
  fix spelling
  Add check for constant in Curl client
  Revert #38614, add assert to avoid regression
  Fix problem when SYMFONY_PHPUNIT_VERSION is empty string value
  Update PHP CS Fixer config to v2.18
derrabus added a commit that referenced this pull request Jan 19, 2021
@derrabus
Merge branch '5.1' into 5.2
31dcefe 
* 5.1:
  [HttpFoundation] Drop int return type from parseFilesize()
  Added $translator->addLoader()
  bug #39878 [doctrine-bridge] Add username to UserNameNotFoundException
  [Uid] Clarify the format returned by getTime()
  fix spelling
  Add check for constant in Curl client
  Revert #38614, add assert to avoid regression
  Fix container injection with TypedReference
  Fix problem when SYMFONY_PHPUNIT_VERSION is empty string value
  Update PHP CS Fixer config to v2.18
derrabus added a commit that referenced this pull request Jan 19, 2021
@derrabus
Merge branch '5.2' into 5.x
d91278a 
* 5.2:
  [HttpFoundation] Drop int return type from parseFilesize()
  Added $translator->addLoader()
  bug #39878 [doctrine-bridge] Add username to UserNameNotFoundException
  [Uid] Clarify the format returned by getTime()
  fix spelling
  Add check for constant in Curl client
  Revert #38614, add assert to avoid regression
  Fix container injection with TypedReference
  Fix problem when SYMFONY_PHPUNIT_VERSION is empty string value
  Update PHP CS Fixer config to v2.18
This was referenced Jan 27, 2021
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@jderusse jderusse jderusse approved these changes

@derrabus derrabus derrabus approved these changes

Assignees
No one assigned
Labels
Bug HttpFoundation Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
8 participants
@mvorisek @fabpot @nicolas-grekas @derrabus @apfelbox @BafS @jderusse @carsonbot