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?
Issue fork ui_patterns-3490150
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
Comment #2
just_like_good_vibeshello,
oups i think i already added things in #3492211: [2.0.0-beta6] Normalization...suite
Comment #3
just_like_good_vibesComment #4
pdureau commentedI have mixed feelings (so an open mind) on this subject:
Comment #5
grimreaperHi,
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.
Comment #6
pdureau commentedLet's imagine we are leveraging contentMediaType and create a new PropType plugin to make the distinction between Markup and PlainText:
type: string; contentMediaType: "text/html"and keeptype: stringfor plain texttype: string; contentMediaType: "text/plain"and keeptype: stringfor markupWhat would be the usage of this Markup prop type? It will be very similar to SlotPropType :
Would it make sense? I don't think so.
Comment #7
just_like_good_vibesi 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
inputfor 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.
Comment #8
pdureau commentedI will have a look
Comment #9
just_like_good_vibeshey 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
Comment #10
pdureau commentedoption 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
contentMediaTypeis stricter thancontentMediaType: "text/html"in JSON schema.Comment #11
pdureau commentedI am now hesitating between option 4
And option 5:
Anyway, with both options, we are going in the right direction.
What do you think?
Comment #13
just_like_good_vibesOption 5
Comment #14
pdureau commentedphpunit fail: https://git.drupalcode.org/project/ui_patterns/-/jobs/3775244
Comment #15
just_like_good_vibesback to it
Comment #17
pdureau commented