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: Replacement for ConvertUris processor #3155

Open
bwaidelich opened this issue Oct 30, 2020 · 1 comment
Open

FEATURE: Replacement for ConvertUris processor #3155

bwaidelich opened this issue Oct 30, 2020 · 1 comment

Comments

@bwaidelich
Copy link
Member

bwaidelich commented Oct 30, 2020

TL;DR see "RFC" part below (but I just realized that it's still quite a mouth full to read..)

The current Neos.Neos:ConvertUris implementation has a couple of issues:

  • It claims that it replaces all occurrences of "node://<UUID>" by proper URIs but in fact it replaces asset URLs too and adds target and noopener attributes to completely unrelated anchor tags
  • It adds a target attribute if the href contains the string "_Resources"
  • It uses Regular Expressions to parse HTML which is almost never a good idea

In general this processor is very critical because it is applied to all inline editable content that is rendered via Neos by default and the current implementation might lead to unexpected output. Additionally the fact that all the functionality is encapsulated within one PHP class makes it really hard to influence the behavior.

As discussed with @jonnitto and @Sebobo I suggest to

  • Fix some issues with the current implementation where possible (see Neos.Neos:ConvertUris should respect exitising relattribute  #2942 for example)
  • Introduce three new Fusion prototypes for the individual requirements (see below)
  • Mark Neos.Neos:ConvertUris deprecated for Neos 6
  • Document how to apply the new prototypes today already
  • Remove Neos.Neos:ConvertUris (or re-wire it to the new behavior) with Neos 7

RFC

I suggest to split up the behavior of ConvertUris into three dedicated Fusion components and extract most of the logic from PHP to fusion in order to make it extensible

Replace node://... URIs

prototype(Neos.Neos:NodeUriProcessor) {
    @class = 'Neos\\Neos\\Fusion\\NodeUriProcessorImplementation'
    value = ${value}
    forceConversion = false

    renderer = Neos.Fusion:Case {
        missingNode {
            condition = ${resolvedNode == null}
            renderer = '#what-to-do' # see questions below
        }
        default {
            condition = true
            renderer = Neos.Neos:NodeUri {
                node = ${resolvedNode}
            }
        }
    }

    @if.isActive = ${this.forceConversion || !node.context.inBackend}
}

Usage

someContent.@process.replaceNodeUris = Neos.Neos:NodeUriProcessor {
    forceConversion = true
}

The resolvedNode would contain the target node of the URL, nodeIdentifier could be made available, too.
So the default behavior would be to replace that with the corresponding URI pointing to the node as before.
But now one could easily extend the behavior:

Create node URI based on the node type

prototype(Neos.Neos:NodeUriProcessor) {
    renderer.sectionNode {
        @position = 'start'
        condition = ${Node.isOfType('Some.Mixin', resolvedNode)}
        renderer = ...
    }
}

Force absolute node URIs

prototype(Your.Site:SocialMediaButtons) {
    prototype(Neos.Neos:NodeUriProcessor) {
        renderer.default.renderer.absolute = true
    }
}

Replace asset://... URIs

Similar to above, this could look like

prototype(Neos.Neos:AssetUriProcessor) {
    @class = 'Neos\\Neos\\Fusion\\AssetUriProcessorImplementation'
    value = ${value}
    forceConversion = false

    renderer = Neos.Fusion:Case {
        default {
            condition = true
            renderer = Neos.Neos:AssetUri {
                asset = ${resolvedAsset}
            }
        }
    }

    @if.isActive = ${this.forceConversion || !node.context.inBackend}
}

Usage

someContent.@process.replaceAssetUris = Neos.Neos:AssetUriProcessor {
    forceConversion = true
}

I can't really think of a realistic scenario that would require a custom behavior here, but I'm sure there are some.
Note: There is no Neos.Neos:AssetUri yet, but there should be IMO :)

Process (external) Links

This one is a bit special: It is about adding rel and target attributes to Links based on the the href.
A flexible implementation could look like this:

prototype(Neos.Neos:LinkProcessor) {
    @class = 'Neos\\Neos\\Fusion\\NodeLinkProcessorImplementation'
    value = ${value}

    renderer = Neos.Fusion:Case {
        assetUri {
            condition = ${resolvedUri.schema == 'asset'}
            renderer = Neos.Fusion:Augmenter {
                rel = 'nopener nofollow'
                target = '_blank'
            }
        }
        externalUri {
            condition = ${resolvedUri.host != request.httpRequest.host}
            renderer = Neos.Fusion:Augmenter {
                rel = 'nopener nofollow'
                target = '_blank'
            }
        }
        default {
            condition = true
            renderer = ${value}
        }
    }
}

Usage

someContent.@process.links = Neos.Neos:LinkProcessor

In this case the renderer would be invoked for every <a href="..." ...>...</a>.
value would contain the full, unmodified, tag string and this would be returned by default.
resolvedUri an UriInterface instance of the parsed href value so that it can be evaluated easily.
For links pointing to external URIs and assets some rel and target attributes would be added via the HTML Augmenter.

Questions / Concerns

  • Names of the new prototypes?
  • Performance should be comparable to the current implementation, but it has to be measured
  • What to do with node://... URLs that can't be resolved (i.e. pointing to a missing/inaccessible node)? Currently the whole link tag is removed in this case
  • The LinkProcessor needs to be applied before the AssetUriProcessor in order to detect asset links by their schema. For node URIs it might be more useful to have them replaced before though to distinguish between nodes of the current site and "external" nodes by the link host.
  • Do we have to make the context variable names configurable (similar to itemName in Loop for example)? and, if so, how to call those props?

Migration

For a starter I would suggest to just add the new Prototypes.
Something like the following snippet could be used in order to replace the current ConvertUri processor:

prototype(My.Custom:ConvertUris) < prototype(Neos.Fusion:Component) {
    value = ${value}
    forceConversion = false
    absolute = false

    renderer = ${value}
    renderer.@process.processNodeUris = Neos.Neos:NodeUriProcessor {
        renderer.default.renderer.absolute = ${props.absolute}
        forceConversion = ${props.forceConversion}
    }
    renderer.@process.processLinks = Neos.Neos:LinkProcessor
    renderer.@process.processAssetUris = Neos.Neos:AssetUriProcessor {
        forceConversion = ${props.forceConversion}
    }
}

prototype(Neos.Neos:Editable) {
    renderer.notEditable.@process.convertUris = My.Custom:ConvertUris
}
@mhsdesign
Copy link
Member

i think its not a good idea to use renderer. as 'public' api for extending the behavior in the shown cases. (we dont do this in 'root' or neos.neos:contentcase either ...)

but moving this behavior into fusion and maybe using the php XML Doc parser sounds like a cool but i think slower? alternative...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

2 participants