Make the payload context an enum and pass it on to nested objects - #24
Merged
Merged
Conversation
The context was a string with two constants on Parameters. A typo such
as getPayload('brwoser') did not fail: it silently returned the server
payload, IP address, user agent, fbc and fbp included, which is exactly
what must not be printed into a page by FbqGenerator::generateInit().
PayloadContext is an enum with the cases Server and Browser.
getPayload() and getMapping() take it instead of a string, and the two
constants are gone.
The context is now also passed on to nested Parameters. Before,
$event->getPayload(BROWSER) still serialized the user data in the server
context. It never showed, because FbqGenerator asks the user data and
the custom data directly.
Subclasses of Event that override getMapping() have to change its
signature. See UPGRADE-2.0.md.
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.
Summary
One of the 2.0 cleanups that was listed in #16 and then never done.
The payload context was a string with two constants on
Parameters. With a string, a typo such asgetPayload('brwoser')does not fail. It silently returns the server payload, IP address, user agent,fbcandfbpincluded, andFbqGenerator::generateInit()prints that payload into the page.Setono\MetaConversionsApi\Event\PayloadContextwith the casesServer(the default) andBrowser.Parameters::getPayload()andgetMapping()take it instead of a string, inEvent,User,CustomandContent.Parameters::PAYLOAD_CONTEXT_SERVERandPAYLOAD_CONTEXT_BROWSERare removed.A fix on the same lines
The context was not passed on to nested objects:
getPayload()called$value->getPayload()on nestedParameterswithout the argument. So$event->getPayload(BROWSER)still serialized the user data in the server context. It never showed, becauseFbqGeneratorasks the user data and the custom data directly. The context now travels down, both for directly nested objects and for lists of them.Backwards compatibility
Intentional breaks, documented in a new section of
UPGRADE-2.0.md:Eventthat overridesgetMapping(string $context)gets a signature mismatch and has to change it togetMapping(PayloadContext $context). The README encourages subclassingEvent; a subclass that only overrides the constructor is not affected.The Roave BC job is red by design.
Docs
README (the browser-side snippet), CLAUDE.md (the
Parametersand payload context paragraphs),UPGRADE-2.0.md.Test plan
ParametersTest(new), with a small context-aware fixture: the server context is the default, and the context reaches a nested object, a doubly nested object and every item of a listEventTest::it_passes_the_context_on_to_the_user_data— the browser payload of an event contains only the hashed email of its user, the server payload contains the IP address, user agent,fbcandfbpas beforeFbqGeneratorTestswitched to the enum, output unchangedNot in this PR
The other two cleanups from the same list are still open:
Fbp::$randomNumberbeing a public, mutable property, and native types forUser::$fbc/$fbpandFb::withCreationTime().