Problem/Motivation

Discussed in #3488582: [2.0.0-beta5] Side effect in links prop type normalization casting with @pdureau, we will not touch StringPropType in the related issue.

But it raised the question if a normalize should be added in it and in this case should it strip tag?

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

grimreaper created an issue. See original summary.

just_like_good_vibes’s picture

hello,
oups i think i already added things in #3492211: [2.0.0-beta6] Normalization...suite

just_like_good_vibes’s picture

Assigned: Unassigned » just_like_good_vibes
pdureau’s picture

I have mixed feelings (so an open mind) on this subject:

  • in a pure JSON Schema point of view, strings with markup are still strings, so no stripping
  • in a SDC (or any similar UI component technology) point of view, markup (already rendered or not) belongs to slots, and string props may be considered as plain text only
grimreaper’s picture

Hi,

I have discussed this issue with @pdureau, if JSON Schema string allows markup, do not strip tags.

But recommend to use a slot if it should contain markup.

pdureau’s picture

Let's imagine we are leveraging contentMediaType and create a new PropType plugin to make the distinction between Markup and PlainText:

  • we can create a Markup prop type with type: string; contentMediaType: "text/html" and keep type: string for plain text
  • or we can create a PlainText prop type with type: string; contentMediaType: "text/plain" and keep type: string for markup

What would be the usage of this Markup prop type? It will be very similar to SlotPropType :

  • accept the same source plugins as SlotPropType: WYSIWYG, Component, Block, Field Formatter...
  • do the same data normalization as SlotPropType, with only one step more: the renderable is renderer

Would it make sense? I don't think so.

just_like_good_vibes’s picture

i find the remark interesting, but still we need to decide what to do :

- Before #3492211: [2.0.0-beta6] Normalization...suite, string prop type was unfiltered. String prop types are supposed to have only string and not tags, but it seems some people were abusing the original intent of prop types and (mistakenly) used them to convey html markup. We were like authorizing this (through twig usage or through weird sources like token ? )

- After #3492211: [2.0.0-beta6] Normalization...suite, which is now : we are stripping tags. Is-it a mistake? should we re-allow tags inside strings? (by the way, tags from forms won't work, like input for example, twig will filter them i guess) ?

Option 1) : we keep the strip tags. I would say then "work as expected". People are not supposed to free-ride string props to put markup in it. They should re-think their SDC component model and use slots for that. The benefit of that strip tags filtering is the ability to have a predictable behavior of string prop types. from a twig usage perspective, people can inject "stuff" (e.g. html) that may be the result of a rendering, and that stuff is cleaned by our normalization process to become nice string.

Option 2) : We remove the strip tags. We open the gate to strange things. but some people would be happy to not rewrite their SDC component, and ui patterns will support their strange usage. still this is not a real html support yet, would we allow also form elements ? (in that case a specific treatment in preprocess should be added).

Option 3) : We keep Option1), but we introduce the new prop type Pierre was talking about. Then, weird usage of string props with tags, would work if and only if SDC component definition would be patched with contentMediaType: "text/html" . That would be a compliance requirement of ui patterns 2. more elegant solution with a nasty new prop type.

Honestly, i like option 3) but it is still an open door to strange things with props. But strange things are going into a specific prop type at least instead of free-riding the nice String prop type.

we need to "vote", for the best option.

pdureau’s picture

Assigned: just_like_good_vibes » pdureau
Status: Active » Needs work

I will have a look

just_like_good_vibes’s picture

hey hey, what about Option 4) we keep the strip tags behavior in string prop types and we do not introduce a new prop type to inject HTML into props. BUT, we implement this : if the JSON schema definition has contentMediaType: "text/html" , then we allow HTML to be placed into the string prop, and the normalization and preprocess on our side behaves in a different manner.

now i vote for option 4

pdureau’s picture

option 4 looks great: no new prop types, no confusion with slots, just an adaptation related to the prop schema.

I keep the issue on my side to check if no contentMediaType is stricter than contentMediaType: "text/html" in JSON schema.

pdureau’s picture

I am now hesitating between option 4

A single StringPropType with type: string JSON Schema, if the JSON schema definition has contentMediaType: "text/html" , then we allow HTML to be placed into the string prop.

And option 5:

A single StringPropType with type: string JSON Schema, if the JSON schema definition has contentMediaType: "text/plain" , then we strip the HTML.

Anyway, with both options, we are going in the right direction.

What do you think?

just_like_good_vibes’s picture

Assigned: just_like_good_vibes » pdureau
Status: Needs work » Needs review

Option 5

pdureau’s picture

Assigned: pdureau » just_like_good_vibes
Status: Needs review » Needs work
just_like_good_vibes’s picture

Assigned: just_like_good_vibes » pdureau
Status: Needs work » Needs review

back to it

pdureau’s picture

Assigned: pdureau » Unassigned
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.