feat!: v2 overhaul, fix the module on stock installs and add stream mode - #4
Merged
Merged
Conversation
composer.json required nothing but PHP, so nothing guaranteed the framework and modules the code actually uses. etc/module.xml declared no sequence. etc/acl.xml was missing entirely, which meant the SamJUK_MediaProxy::config resource referenced by the admin form could not be granted to a role.
The module could not run on a stock install. RequestedMedia asked the object manager for Magento\Framework\Filesystem\DriverInterface, which Magento never binds, so every media miss raised an uncaught Error. It only got past that on installs where a third party happened to declare the preference globally, and there cache mode created a directory where the image should have been, on every miss, leaving pub/get.php to 404 on it forever after. Media resolution now comes from the HTTP request rather than $GLOBALS['relativePath'], is normalised and bounds checked against the media directory, and writes go to a temporary file that is renamed into place so concurrent requests cannot serve a partial image. Failures are logged and fall through to Magento, so a miss serves the usual placeholder instead of an error. Upstream responses are validated before anything is written. A Magento upstream answers a request for media it does not hold with its own placeholder and a 200, so the module learns that signature once a day from a probe path that cannot exist and refuses anything matching it. Failed lookups are remembered for five minutes, which stops a page of missing images re-asking the upstream on every render. Downloads are capped at 32MB, because buffering more exhausts the memory limit and that fatal cannot be turned into a graceful failure. Adds a third mode, stream, which fetches through PHP and keeps nothing. It is the answer for an upstream behind HTTP basic auth, since proxy mode redirects the browser, which has no credentials. Credentials go through CURLOPT_USERPWD rather than a literal Authorization header, which curl replays across a cross host redirect, and are stripped from anything written to the log. Every upstream request carries X-SamJUK-Media-Proxy and is refused on the way back in, so an upstream that resolves to this installation can no longer recurse through PHP-FPM until no workers are left. Config is read at store scope, so the website scope the admin form offers is no longer silently ignored, and gains a timeout and upstream credentials. BREAKING CHANGE: ConfigInterface gains isStreamMode(), getTimeout(), getUpstreamUsername() and getUpstreamPassword(). RequestedMediaInterface gains getUpstreamRedirectUrl() and download(). The constructors of Model\Config, Model\RequestedMedia and Plugin\Media have all changed. Model\Config\Source\Mode now implements Magento\Framework\Data\OptionSourceInterface instead of extending Magento\Eav\Model\Entity\Attribute\Source\AbstractSource, dropping an undeclared Magento_Eav dependency, and no longer has getAllOptions().
_local_test.sh could not run at all. It read a CI matrix key removed in ad13806, built image tags as 8.3-magento2.4.8 when the publisher had moved to php83-fpm-magento2.4.8, and the image no longer ships make. Versions can now be overridden with VERSIONS="8.3:2.4.8" to test a single combination. The Makefile's default RELATIVE_PROJECT_DIR was one level short for the app/code/<vendor>/<module> layout it documents, and test-integration ran the whole Magento suite rather than this module's.
SECURITY.md had no statement of scope. The module fetches from an admin configured host and follows redirects, which is worth saying out loud.
Magento 2.4.9 and Mage-OS 3.5.0 ship PHPUnit 12, which dropped support for the @dataProvider annotation. The provider was not applied, so the test ran with no arguments and raised ArgumentCountError, taking the suite from 89 tests to 86 with one error. Both forms are declared because the supported range spans PHPUnit 9 through 12. The annotation is what 9 reads, 2.4.3 to 2.4.6 pin it and it has no attribute support. The attribute is what 12 reads, and on the PHP 7.4 that 2.4.3 runs on it is parsed as a comment. Verified 89 tests and exit 0 on PHPUnit 9.6, 10 and 12, and that PHP 7.4 still parses the file. require-dev widened to match, since the module is exercised against all four majors rather than only the 10.5 it claimed.
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
1 participant
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.
Breaking. Intended as
v2.0.0.Why
The module could not run on a stock install.
RequestedMediaasked the object manager forMagento\Framework\Filesystem\DriverInterface, which Magento never binds (it passes it explicitly per type), so every media miss raised an uncaughtError. It only got past that on installs where a third party declared the preference globally,amasty/basebeing the common one. There, cache mode created a directory where the image should have been, on every miss including ones where the upstream was unreachable, andpub/get.php404s on a directory forever after.Both reproduced against the
v1.0.1tag in a clean 2.4.8 container.Fixes
$GLOBALS['relativePath'], and is normalised and bounds checked against the media directory.etc/acl.xml, the composer requires and the module sequence.New
streammode: fetches through PHP and keeps nothing. This is the answer for an upstream behind HTTP basic auth, since proxy mode redirects the browser, which has no credentials.200, which is indistinguishable from a real image. The module learns that signature once a day from a probe path that cannot exist, and refuses anything matching it.Security
Authorizationheader ontoCURLOPT_USERPWD. curl replays custom headers across a cross host redirect; it scopesCURLOPT_USERPWDto the first host.X-SamJUK-Media-Proxyand is refused on the way back in, so an upstream that resolves to this installation can no longer recurse through PHP-FPM until no workers are left.Breaking changes
ConfigInterfaceandRequestedMediaInterfaceboth gained methods, and the constructors ofModel\Config,Model\RequestedMediaandPlugin\Mediaall changed.Model\Config\Source\Modenow implementsMagento\Framework\Data\OptionSourceInterfacerather than extending the EAVAbstractSource, dropping an undeclaredMagento_Eavdependency and losinggetAllOptions().Installs that ran 1.x cache mode have empty directories shadowing images. The changelog has the cleanup one-liner.
Verified
di:compilegreen on 7.4/2.4.3, 8.1/2.4.4, 8.1/2.4.5, 8.2/2.4.6, 8.3/2.4.7, 8.3/2.4.8 and 8.4/2.4.8.v1.0.1to this upgrade rehearsed end to end, including the documented cleanup.