Use the default front controller for URLs generated outside a web request (#25976) - #41172
Open
lbajsarowicz wants to merge 5 commits into
Open
Use the default front controller for URLs generated outside a web request (#25976)#41172lbajsarowicz wants to merge 5 commits into
lbajsarowicz wants to merge 5 commits into
Conversation
…O using Console Command
…uest (magento#25976) With "Use Web Server Rewrites" disabled, Store::_updatePathUseRewrites() appended basename(SCRIPT_FILENAME) to the base URL, which under CLI is the console binary - producing links such as .../magento/catalog/... . Fall back to index.php whenever the running script is not a .php entry point, which also covers an empty or missing SCRIPT_FILENAME.
Contributor
Author
|
@magento run all tests |
|
Hi @lbajsarowicz. Thank you for your contribution!
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
Open
5 tasks
Contributor
Author
|
@magento run Unit Tests |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
With Use Web Server Rewrites disabled, URLs generated outside a web request get the console binary as their entry point —
https://example.com/magento/catalog/...instead ofhttps://example.com/index.php/catalog/.... This affects anything that builds URLs from the CLI: transactional emails sent by cron, sitemap generation, indexers, custom commands.Magento\Store\Model\Store::_updatePathUseRewrites()takes the entry point straight from the running script:Under
bin/magentothat basename ismagento, which is not a front controller at all.Fix
Rather than trying to detect the execution context, this validates the result: if the resolved entry point is not a
.phpscript it cannot be a web front controller, so fall back toindex.php— exactly the value the_isCustomEntryPoint()branch two lines above already produces.A
.phpfilename is passed through byte-for-byte, so every real web request —pub/index.php,pub/get.php,pub/static.php,pub/cron.php, and any custom entry point — is untouched. This also fixes the empty case, which previously emitted a double slash (https://example.com/link//).That
index.phpis the right answer here is already encoded in the test framework:dev/tests/integration/framework/Magento/TestFramework/Bootstrap/Environment.php:24fakesSCRIPT_FILENAME = 'index.php'precisely because the real CLI value is unusable, andMagento/Framework/UrlTest.php:45assertshttp://localhost/index.php/.Why not detect the CLI directly
PHP_SAPI === 'cli'looks like the obvious fix and there is core precedent for it, but it is wrong for this method: the unit and integration suites all run under thecliSAPI while deliberately emulating web requests.StoreTest::testGetBaseUrlEntryPointand the integrationtestGetBaseUrlForCustomEntryPointboth assert web behaviour from a CLI process, so a SAPI check would break them and make the branch untestable.Setting
Store::CUSTOM_ENTRY_POINT_PARAMfrom the console entry point was the other candidate. The natural hook isMagento\Framework\Console\Cli, but referencingMagento\Store\Model\Storefromlib/internal/Magento/Frameworkis forbidden by the library dependency static test, and doing it inbin/magentomeans writing to$_SERVER. It would also fix nothing for third-party scripts that bootstrap Magento themselves.This PR continues #34639
Rebased onto current
2.4-develop, preserving the commits of @rmsundar1 and @engcom-Bravo.One correction worth recording, since it is why the original stalled. @ihor-sviziev's "It doesn't look real example" was right: the branch tested
SCRIPT_FILENAME !== 'bin/magento', a strict comparison against a relative literal, while the real CLI value is an absolute path such as/var/www/html/bin/magento— so the condition never matched and the fix never fired. That comparison was not @rmsundar1's original approach; his commit usedgetOriginalPathInfo()as the "not a web request" signal (right instinct, wrong signal — it is legitimately empty for a homepage request), and the literal was introduced later while chasing test failures. Both are replaced here.Fixed Issues
Fixes #25976
Manual testing scenarios
bin/magento sitemap:generate./magento/. After: links contain/index.php/./index.php/...from a normal web request).custom_entry.php) and confirm it is still used verbatim for web requests.Questions or comments
Gates run locally on
2.4-develop(Warden, PHP 8.3):Store/Test/Unit/Model/StoreTest.php— 49 tests pass, including a new six-case data provider covering a default web entry point, a custom web entry point, an absolute CLI path, the custom-entry-point flag, and empty/missingSCRIPT_FILENAME.Magento/Store/Model/StoreTest.php+Magento/Framework/UrlTest.php— 96 tests pass. These are the ones that emulate web requests from a CLI process, so they are the real regression check for this change.Magento2: clean. PHPStan level 1: no errors.Store.php(yieldingmagento/and//) and pass with the fix; the other three pass both before and after, pinning the no-change guarantee for web requests.No signature changes —
Storestays@api-compatible, with no new constructor parameter or DI wiring.One theoretical case for a second opinion: hosting where the front controller has no
.phpextension would now getindex.phpinstead of that name. I could not construct such a setup for Magento (all shipped entry points are.php, and rewrite-based extensionless URLs still report the.phpfile inSCRIPT_FILENAME), andindex.phpis the correct front controller there anyway — but it is the one behaviour change outside CLI beyond the empty-value case.Contribution checklist