Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Part of #2549943: [plan] Remove as much of the SafeMarkup class's methods as possible. The only use of drupal_placeholder() and SafeMarkup::placeholder() in core are wrong. They are basically using it to wrap with em
tags.
Proposed resolution
Remove SafeMarkup::placeholder(), deprecate drupal_placeholder() and stop drupal_placeholder() from marking safe.
Remaining tasks
- Agree
- Patch
- Commit
User interface changes
None
API changes
- Remove SafeMarkup::placeholder()
- Deprecate drupal_placeholder()
- Don't mark output drupal_placeholder() of safe
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#45 | 2552579.45.patch | 12.11 KB | alexpott |
#41 | 2552579.41.patch | 12.24 KB | alexpott |
#41 | 36-41-interdiff.txt | 3.91 KB | alexpott |
#36 | 33-35-interdiff.txt | 747 bytes | alexpott |
#36 | 2552579.35.patch | 11.76 KB | alexpott |
Comments
Comment #2
alexpottComment #3
catchJust nits, looks lovely.
'does not' isn't it?
Are there any valid use cases?
So just no
<em>
tags here?Comment #4
dawehnerDid the 'class="placeholder"' added better accessibility?
Comment #5
alexpott@dawehner I'm not sure how the
placeholder
class is more accessible?@catch thanks for the review.
1. Yep it does not. Also I've improved the docs to point to SafeMarkup::format().
2. Yep no
<em>
- why to why need emphasis of a dash?Comment #6
stefan.r CreditAttribution: stefan.r commentedDoes $base_theme lose the emphasis here?
* - %variable: Escaped to HTML and formatted using self::placeholder()
Not anymore :)
Comment #7
dawehnerSadly #601548: Loosen the dependency between t() and the theming layer never had a justification for the class so yeah I think its totally fine to remove it.
Comment #8
alexpott@stefan.r fix the docs.
This keeps the placeholder markup in the update report.
HEAD
PATCH
So yep the patch put more inside the
<em>
tag - but is that a bad thing - tbh I think the list of dependencies should not contain the update required text?Ah!!!! This base theme stuff in the update report is legacy stuff. In D7 days base theme did not have to be enabled to this update report had to do special things. In D8 they do so this is all legacy.
Comment #9
alexpott#2338167: Update ProjectInfo class to reflect changes to extension system makes all the proper changes to the update report.
Comment #10
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI think the justification is at #601548-7: Loosen the dependency between t() and the theming layer. But the cases that this patch removes it from look like they are probably all OK without it (seems good as long as it stays for actual translation placeholders).
This doesn't look right since it's causing the whole row to be italicized (including things like the "Configure" button).
Also the addition of the fallback_format class is causing the drag and drop handler to disappear from that row. No idea why...
Comment #11
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedOh I see, I think it's because FilterFormatListBuilder::buildRow() does this at the bottom:
That is not going to merge things like $row['#attributes']['class'] correctly, so it winds up without the "draggable" class.
Comment #17
alexpottFixed up the test fails.
@David_Rothstein yep I noticed that too - I asked @joelpittet on IRC about the best way to address it. But in lieu of an answer I think the best thing to do will be to just make the roles text italic for fallback formats since that actually makes semantic sense. That's what we actually want to emphasise.
Comment #18
joelpittet+1 lol, I'm sure that made a difference in Arial or most sans-serifs.
Label looks as though it needs a wrapper attribute class too.
This will wrap em around both the label and the base theme. So the end result won't be the same.
Should the data still be escaped? or the new use statement not needed?
+1 Totally!
Comment #19
alexpott@joelpittet thanks for the review:
Comment #21
joelpittet@alexpott #18.3 is silly how? it make a visual distinction between the two elements to me, there could be a better way to do that, but I don't think it's silly. Maybe those em tags could be moved into the template in a foreach loop instead of the join we are currently doing.
Comment #22
alexpott@joelpittet it is silly when you actually look at the output and image a theme that depended on multiple base themes then you'd have this italic non-italic thing going on for little gain in legibility or semantic meaning. But, anyway, as I pointed out this is moot this all of this now completely and utterly unnecessary since base themes have to be enabled. This only existed because in D7 they didn't have to be enabled.
The violation change is a bit interesting. The common form required text for translators is
!name field is required.
so I'm pretty sure the intention was to not make the translators have to translate yet another string. So we need to escape the field label and use the pass-through notation. Added a comment about this.Comment #23
joelpittetOk let's get some screenshots on the bits that have changed markup:
Filter Format List
Project status
And keep in mind that
<em>
may not be represented the same as<em class="placeholder">
as it has a purpose and should be taken into careful consideration if it's being removed, as it may be purposefully visually strengthened by it's CSS class.Comment #24
alexpottScreenshots of the change to the update status report are in comment #8. Which will disappear anyway because this information is irrelevant now that base theme have to be installed.
And grepping the codebase for .placeholder in css shows that no of the places where it is used is affected by this change.
Filter screenshots:
HEAD
Patch
The screenshots for the filter listing page show how inappropriate the placeholder class is here - like why is the fallback placeholdered and the other not. The only reason is to mark the fallback format as something special. It has nothing to do with the placeholders. As stated about I think just emphasising the role text is enough because (a) this is not a role so it is different to the other values in this column (unlike the other columns) and (b) it is the special information that the table is trying to communicate is special.
Comment #25
joelpittetI completely agree regarding the Filter Admin thank you for the screenshots on the Filter admin, those really help.
Comment #26
joelpittetAlso, confirm there is nothing in this patch that is affecting the
.placeholder
CSSComment #27
joelpittetStill a bit unclear and not sure where the irreverence around the basethemes is found. It's not popping out of the patch for me as clear as it is to you.
Comment #28
alexpott@joelpittet the irrelevance of base themes to the update status report is this. In D7 base themes didn't have to be installed so they would not have a regular project entry on the update status report. Now they do because they have to be installed. The special base theme and sub theme handling can be completely removed from the update status report, which is shown by the patch in #2338167: Update ProjectInfo class to reflect changes to extension system. A base theme is now just a regularly installed theme.
Comment #29
Wim LeersLooks great! Found only one problem:
This seems wrong? That library is solely for filter settings, not for a listing of formats.
Removed the #24 added screenshots.
tag,Comment #30
alexpott@Wim Leers good point. Now we're only making the role description emphasised we don't actually need this. Given that we were wrapping this in SafeMarkup::placeholder() just using #prefix and #suffix does not represent a regression.
Comment #31
Wim LeersDo we want to block this on #2338167: Update ProjectInfo class to reflect changes to extension system or not?
Comment #32
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedThis docblock left me a bit confused.
- in "an" associative array
- to make what?
- "this this"
- "few valid use-cases": the method is deprecated, so what valid use case is left? We should always implement an alternative, right?
The 3rd element should probably be "#suffix".
Comment #33
alexpottRerolled now #2338167: Update ProjectInfo class to reflect changes to extension system has landed.
Re #32
%placeholder
or theplaceholder
filter in Twig. tbh I'd remove the method but we're very close to RCComment #34
Wim LeersContrary to what #33.2 says, this still appears to be here?
s/emphasise/emphasize/
(I know, I know, British English is more elegant, but alas.)
NW for point 1.
Comment #35
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedRe #33
1. Looks good!
2. This particular line wasn't removed, so it's still in your latest patch.
Comment #36
alexpottalexpott--
Addressed #34
Comment #37
DuaelFrdrupal_placeholder() was going to be deprecated by #2221771: Mark all simple wrappers in bootstrap.inc and common.inc as deprecated.
As there is a better comment in this patch and as deprecating that function here makes that issue more consistent, I asked to remove that deprectation from the other issue.
Comment #38
Wim Leers#36's interdiff is wrong; it only lists #34.2 being fixed, but #34.1 is also fixed :)
Comment #39
stefan.r CreditAttribution: stefan.r commentedTests are green, code looks good and grepping for ::placeholder, drupal_placeholder and placeholder shows nothing funny, so RTBC+1, just nits:
wonky grammar here
"a %variable" also reads odd
Is this still true?
SafeMarkup class is unused here
Unrelated to this patch, but this implode and others could be candidates for using the CSS-based comma separated item_list in a followup.
this is on multiple lines in one chunk of code but on 1 line elsewhere, let's do multiple lines everywhere?
Comment #40
alexpottDoing #39
Comment #41
alexpottFixed everything that needed fixing.
Very much so - this patch does not remove placeholder support from SafeMarkup::format or t() at all. And the contents are still escaped. No change here.
Comment #42
stefan.r CreditAttribution: stefan.r commentedYes i saw that when reading the code, its just that the wording was unclear -- we used to call the placeholder function specifically. It's fine like this anyway.
Comment #45
alexpottA simple conflict in SafeMarkupTest cause of #2550055: Remove SafeMarkup::replace() and use ViewsRenderPipelineSafeString instead
Comment #46
catchCommitted/pushed to 8.0.x, thanks!
Comment #48
joelpittet@alexpott thanks for trying to explain in #28 I guess a picture would have been a bit more useful to me, or to thick to understand.
Comment #49
xjmhttps://www.drupal.org/node/2302363 did not get updated for this change; we need to make sure to update previous change records as well providing new ones.
Comment #50
xjmI made this update:
https://www.drupal.org/node/2302363/revisions/view/8787151/8837711
Comment #51
xjmSorry, it's fixed now. :)