Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FEATURE: Add additionalParams to ConvertUrisImplementation #3486

Closed

Conversation

jonnitto
Copy link
Member

@jonnitto jonnitto commented Oct 30, 2021

If you create newsletter with Neos, it is crucial to have some additional parameters like utm_source, utm_medium and utm_campaign to every link. This change enables the ConvertUrisImplementation to have additionalParams.

prototype(Neos.Neos:ConvertUris) {
    absolute = true
    forceConversion = true
    additionalParams {
         utm_source = 'newsletter'
         utm_medium = 'email'
         utm_campaign = ${node.label}
    }
}

@bwaidelich
Copy link
Member

We mix that in other parts already, but additionalParams != query arguments.
IMO we should keep the routing part to the UriBuilder and don't mess with the resolved URI "manually".
Depending on your routing setup the resolved URL could already contain query arguments so appending additional ones with ?... would not work.
Instead we should probably extend LinkingService::resolveNodeUri() such that it can accept additional arguments that it passes to

public function createNodeUri(ControllerContext $controllerContext, $node = null, NodeInterface $baseNode = null, $format = null, $absolute = false, array $arguments = [], $section = '', $addQueryString = false, array $argumentsToBeExcludedFromQueryString = [], $resolveShortcuts = true): string

I also wonder if those arguments are always to be appended to node and asset links.

@markusguenther
Copy link
Member

Something new about this PR? It seems like an ongoing discussion :)

@markusguenther
Copy link
Member

Can we find a solution here? As we have feature freeze tomorrow ;)

@bwaidelich
Copy link
Member

bwaidelich commented Nov 22, 2021

Can we find a solution here?

I still stand to my suggested solution (extend LinkingService::resolveNodeUri()) but it would require proper testing and I can't take care of this atm.

To work around the limitation (i.e. to achieve what @jonnitto described in the original PR message) I would suggest to add a separate processor that just appends query parameters to already converted URLs:

@process.convertUris = Neos.Neos:ConvertUris
@process.addNewsletterQueryParams = Some.Package:AddQueryParamsToLinks {
  params {
    utm_source = 'newsletter'
    utm_medium = 'email'
    utm_campaign = ${node.label}
  }
}

@jonnitto
Copy link
Member Author

I think, it won't make it into 7.3 anyway, but I will look to bring it into Neos 8 with the extended LinkingService

@markusguenther
Copy link
Member

Are you able to work on that for 8.3?

@bwaidelich
Copy link
Member

This might be related to neos/flow-development-collection#2744

@bwaidelich
Copy link
Member

..and maybe #3914

@mficzel
Copy link
Member

mficzel commented Feb 24, 2023

Not sure about this, i would rather use a custom processor for that rather than extend convertUris.

@process.convertUris = Neos.Neos:ConvertUris
@process.addTrackingParams = Vendor.Site:AddTrackingParameters {
    utm_source = 'newsletter'
    utm_medium = 'email'
    utm_campaign = ${node.label}
}

Could be distributed as package for reusability so imho there is no need to support this directly from the core.

Just seen that Bastian already suggested a custom processor for that. So i second that

@mhsdesign mhsdesign changed the base branch from 8.2 to 8.3 February 8, 2024 12:54
@github-actions github-actions bot added the 8.3 label Feb 8, 2024
@Sebobo
Copy link
Member

Sebobo commented Jul 27, 2024

@jonnitto you want to take another try at this or should we close it?

@jonnitto jonnitto closed this Aug 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants