fix: the image_scales original download points at the original again - #37
Merged
Conversation
Half of #15, and the half where "should this go through Thumbor at all?" answers itself. ImageFieldScales.get_original_image_url builds the metadata's top-level download by asking for a scale at the original's *own* dimensions. Under Thumbor that came back as a Thumbor URL, which is wrong twice over. It is not the original. A 1:1 request through an image processor is at best a re-encode, and since 0.7.0 a Thumbor URL names the source derivative — so a "download original" link handed over a capped, colour-converted rendition rather than the uploaded bytes. On the press site this package was built for, that is precisely the asset the link exists to serve. And it is not context-relative, which the metadata contract requires. _scale_view_from_url strips the context URL off and the renderer puts it back; with PGTHUMBOR_SERVER_URL=/thumbor there is no context prefix to strip, so the stored value was "thumbor/<signed>" and the renderer emitted "{image_url}/thumbor/<signed>". Broken for every consumer of the column, including plone.namedfile's own tag(). The route to serve it properly already existed. @@images/{fieldname} returns the original under pgthumbor today: the field branch of ImageScaling.publishTraverse builds the scale view without a uid, and ThumborImageScale.__init__ only produces a Thumbor URL when one is present, so index_html() falls through and Plone streams the stored bytes. _skip_type_fallback_url already builds exactly that URL for SVG, so this shares it rather than spelling it a second time. Registered for IPlonePgthumborLayer, which is strictly more specific than plone.namedfile's (field, content, Interface), so it wins the lookup without an overrides.zcml entry and sites without the add-on keep the stock adapter. Per-scale downloads are untouched, and a test pins that only the one method is overridden. Those *should* be Thumbor URLs; their own version of the host-root problem is the rest of #15 and needs answering together with #7, since absolute URLs would fix the rendering but bake in the host that the relative setting exists to avoid. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Half of #15 — the half where "should this go through Thumbor at all?" answers itself.
ImageFieldScales.get_original_image_urlbuilds the metadata's top-leveldownloadby asking for a scale at the original's own dimensions. Under Thumbor that came back as a Thumbor URL, and that is wrong twice over.It is not the original. A 1:1 request through an image processor is at best a re-encode — and since 0.7.0 a Thumbor URL names the source derivative, so a "download original" link handed over a capped, colour-converted rendition rather than the uploaded bytes. On the press site this package was built for, that is precisely the asset the link exists to serve.
It is not context-relative, which the metadata contract requires.
_scale_view_from_urlstrips the context URL off and the renderer puts it back. WithPGTHUMBOR_SERVER_URL=/thumborthere is no context prefix to strip, so the stored value wasthumbor/<signed>and the renderer emitted{image_url}/thumbor/<signed>— broken for every consumer of the column, includingplone.namedfile's owntag().The route already existed
@@images/{fieldname}returns the original under pgthumbor today. The field branch ofImageScaling.publishTraversebuilds the scale view with onlydata=andfieldname=, without auid, andThumborImageScale.__init__only produces a Thumbor URL when one is present — so_thumbor_urlstaysNone,index_html()falls through to the parent, and Plone streams the stored bytes._skip_type_fallback_urlalready builds exactly that URL for SVG (#17), so this shares it rather than spelling "the original's own URL" a second time. A test pins that.Registration
For
IPlonePgthumborLayer, which is strictly more specific thanplone.namedfile's(field, content, Interface), so it wins the lookup without anoverrides.zcmlentry — and sites without the add-on keep the stock adapter.Deliberately not in scope
Per-scale
downloadentries are untouched, and a test pins that only the one method is overridden. Those should be Thumbor URLs. Their own version of the host-root problem is the rest of #15 and wants answering together with #7: absolute URLs would fix the rendering, but bake in the host that the relative setting exists to avoid.So #15 stays open after this.
595 tests green,
adapters.pyat 100%.🤖 Generated with Claude Code