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

[V14] Make the backend work with the new localLinks format #16661

Merged
merged 3 commits into from
Jul 2, 2024

Conversation

Migaroez
Copy link
Contributor

@Migaroez Migaroez commented Jun 27, 2024

Description

The new backoffice has removed UDI notation from the locallinks format in RTE tags and split up the type and key into 2 attributes (href and type).

The backend was not fully updated and so when rendering the content trough razor pages, or retrieving trough the content delivery api, the locallink notation was not properly replaced by the actual content/media urls.

This PR aims to fix this and update/improve the unittests along the way.

The old format was not migrated to the new format in the v14 migration and this is currently not changing as the backend can handle both.

Testing

Consider both RTEs on a document type and RTEs in block
Both migrated and new data should work as expected
Both Razor rendering and delivery api parsing should create similar output when comparing v13 and v14 data, both migrated and new, when it comes to v14.
The backoffice should allow editing of both old and new formatted data.
Non Local links should still work.

Notes

There is a decent amount of duplicated code, I chose not to refactor this as we should obsolete/remove it at some point anyway. This will require some form of migrating the old formatted data.

@ronaldbarendse
Copy link
Contributor

What was the reason for changing the local link format/syntax in the first place? I don't believe there's a migration that parses all content and changes the old format to the new one (so the pickers in the backoffice also work as expected - or do they still support the UDI format?). And even if the CMS had a migration, this format is also used outside content, like in Forms (for the 'Message on submit' and header/footer text in HTML emails).

I do know that the Management API uses GUIDs instead of UDIs to make parsing of the returned JSON easier (as most libraries have parsers/converters for GUIDs), but the whole reason for UDIs was to have a single value that contains both entity type and ID (potentially also culture and segment). I'd argue we should change the RTE in v14 to use the UDI value in local links again, as that restores compatibility and removes the need for parsing multiple HTML attributes (that need to be in a specific order for the regex to work as well) 😄

@Migaroez
Copy link
Contributor Author

Migaroez commented Jun 28, 2024

The reason for the UDI format to be removed is because it should be an internal only format, at least that's what I remember from the discussions with @kjac?

(that need to be in a specific order for the regex to work as well)

Not really, check out https://regex101.com/r/BBsHNb/2 It supports any order and any irrelevant attributes.

@bergmania
Copy link
Member

@Migaroez

Code looks good and tests out great, but I'm wondering if the type=\"document\" should be removed in the output when translated to a link if we understand it. As far as I can see https://www.w3schools.com/tags/att_a_type.asp type is expected to be a mimetype

@bergmania bergmania merged commit 46acd51 into v14/dev Jul 2, 2024
13 of 14 checks passed
@bergmania bergmania deleted the v14/fix/rte-local-links branch July 2, 2024 12:22
@bergmania
Copy link
Member

I merged, but let me know if you also thing we should consider what I mention above. We can create a separate task in that case

@Migaroez
Copy link
Contributor Author

Migaroez commented Jul 2, 2024

Code looks good and tests out great, but I'm wondering if the type=\"document\" should be removed in the output when translated to a link if we understand it. As far as I can see https://www.w3schools.com/tags/att_a_type.asp type is expected to be a mimetype

Yeah sounds like a good idea, I will make a task to go with the other follow up tasks of this one.

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.

Rich text property editor generates incorrect markup for local links
4 participants