refactor: allow extending Typo3Integration - #127
Open
contemas-tschmidt wants to merge 1 commit into
Open
Conversation
The class declares setUrl() and getServerRequest() as protected, which only has an effect for subclasses -- but the class is final, so there can be none. Dropping final makes that intent usable. Concretely it allows a project to override setUrl() while an upstream fix for a request-related problem is pending, instead of reimplementing the whole integration and swapping it into the integrations option. 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
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.
Typo3Integrationdeclares two of its methodsprotected:protecteddiffers fromprivateonly for subclasses, andfinalmeans there can be none — so the visibility currently has no effect. This dropsfinalso that it does.Why it matters in practice
While #121 / #124 are open, the obvious downstream workaround is a four-line subclass that guards
setUrl()against a request without a URI. That is not possible, so the alternative is to reimplement the integration and replace the class in theintegrationsoption — which then silently drifts from upstream whenever this one changes.We learned this the hard way: the subclass looked correct, and
finalturned it into a fatal error on class load that took down a deployment.finalhere does not prevent a bad idea, it only makes the good version of it unavailable.UserIntegrationhas no protected members andIgnoreEventIntegrationis not affected by this, so this PR touches only the class where the intent is already visible in the code.Scope
One line.
processEvent()staysprivate— making itprotectedwould be a larger decision about what counts as the extension surface, and this change does not depend on it.Not a replacement for #124: that PR fixes the actual bug and should still be merged. This one is about what downstream can do in the meantime, and about extension points in general.