Problem/Motivation
The SafeMarkup::format() use in the LinkGenerator is a bad example. Let's ditch it.
$result = SafeMarkup::format('<a@attributes>@text</a>', array('@attributes' => new Attribute($attributes), '@text' => $variables['text']));
This type of markup building where attributes is just being shoved is wrong. Also at the moment hook_link_alter() can change $variables['text'] without being concerned with safeness which is a security bug waiting to happen. The hook was introduced in #2047619: Add a link generator service for route-based links
Proposed resolution
Make GeneratedLink a SafeString. This has many advantages as we no longer have Drupal::l() returning a string sometimes and an object others. It always returns an object.
This issue also unblocks #2559971: Make SafeMarkup::format() return a safe string object to remove reliance on a static, unpredictable safe list
Remaining tasks
Review- Commit
User interface changes
None
API changes
LinkGenerator::generate()
always returns a GeneratedLink.
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#39 | 2568977-39.patch | 30.79 KB | alexpott |
#39 | 35-39-interdiff.txt | 2.29 KB | alexpott |
#35 | increment.txt | 1.2 KB | pwolanin |
#35 | 2568977-35.patch | 30.69 KB | pwolanin |
#31 | interdiff.txt | 5.97 KB | Wim Leers |
Comments
Comment #2
alexpottComment #3
dawehnerCan we have at least one dedicated
assertInstanceOf
?Comment #5
alexpott@dawehner we can check every link asserted against :)
The fail in #2 is fun.
Comment #9
dawehnerWhat I wonder whether we can consider SafeString still as internal then. The link generator is not really part of the rendering system, as we see in the namespace
Comment #10
alexpottSo how about this then...
Comment #11
dawehnerIt would be nice if this works!
Comment #12
dawehnerNice solution!
Comment #13
dawehnerBlocks #2569281: Change translatable string with !placeholder AccountForm to use a URL instead of a generated link and provide better context
Comment #14
dawehnerThis was a bit premature, I think
Comment #16
fgmTiny commenting error, I think.
This is now always a GeneratedLink, so comment needs to be fixed.
Comment #22
alexpottHmmm... yet another untested hook... hook_link_alter(). This patch now closes a security hole in hook_link_alter().
Comment #23
alexpottComment #24
Wim Leers:O I've never even heard of that hook.
Alex Pott pinged me:
So, +1 to removing
$collect_bubbleable_metadata
, and just always doing it. It simplifies the Link generation API.Comment #25
alexpottComment #26
alexpottSo should we do the
$collect_bubbleable_metadata
removal in this issue? One argument for is that it keeps it consistent with HEAD - if you get a generated link from the generator it has the metadata there. One argument to not do that is scope. Thoughts?Comment #29
Wim LeersThis is what makes it IMHO within scope: the reason this
$collect_bubbleable_metadata
parameter existed, was to get different return values.If that's no longer the case, there's very, very little remaining value to keeping it a separate parameter, and IMHO is mainly just unnecessarily confusing.
Comment #30
alexpottSo yet again this current crop of
SafeMarkup::format()
find more bugs. This time in FieldPluginBase::renderAsLink(). That is concatenating markup together without a care in the world for safeness! The correct fix here is to change that to a render array.This patch also removes
$collect_bubbleable_metadata
from the LinkGenerator.Comment #31
Wim LeersDocblock not updated.
Nit: s/urls/URLs/
So much better! :)
Aha, this is how you got around the fact that the Views render pipeline is string-based, i.e. this is why you are able to use a render array above.
Makes sense. And is consistent with prior behavior wrt bubbling: the bubbling is now done by the renderer, previously it was done by the link generator.
Could be simplified to:
because in other places (last quoted bit), that is done too.
Updated.
Addressed all my nits, which is… all my feedback. This looks great.
Comment #32
dawehnerCould we add a follow up to not have to deal the render() call in viewsTokenReplace()? Now that we have a render array we would not longer need that additional render() call
Comment #33
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia for YesCT commentedTaking a look at this
Comment #34
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia for YesCT commentedFollow-up #2571343: In Views, FieldPluginBase::renderAsLink() should return a render array, not a string
Comment #35
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia for YesCT commentedThese changes are very welcome in terms of removing a WTF from the api:
This patch just removes a unused "use" and fixes the doc on one return value.
Otherwise looks good.
Comment #36
alexpottComment #37
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedAdded a draft change record
Comment #38
znerol CreditAttribution: znerol commentedFound only nits:
Great! Should also remove @param docs for the removed parameter.
s/ is / it /
This is a bit difficult to understand especially ... because $value_is_safe is safe has been used ....
Comment #39
alexpott@znerol good points. Addressed them.
Comment #41
effulgentsia CreditAttribution: effulgentsia at Acquia commentedNice improvements. Pushed to 8.0.x. Some questions for either answering here, or if needing a patch or discussion, can be follow-ups:
Why are we casting $this->generatedLink to a string? When is it not that already? Also, should we now deprecate the getGeneratedLink() method and instruct people to cast the GeneratedLink object to a string instead?
Whoa. Scary that GeneratedLink implements SafeStringInterface and also has a public setter. Should we add an @internal to setGeneratedLink() and some other docs explaining that the caller is responsible for security?
Also, the CR looks like a good start, but I didn't publish it yet, because it doesn't mention that GeneratedLink now implements a __toString(), so most calling code should be unaffected, but invocations of PHP functions that treat scalar strings and stringifiable objects differently would be affected. I think that would be good info to add so people don't think the change is bigger than it really is.
Comment #42
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThis possibly broke #2571593: Fatal error: Cannot use object of type Drupal\Core\GeneratedLink as array in Tableselect.php on line 220
Comment #43
effulgentsia CreditAttribution: effulgentsia at Acquia commentedOnce #42 is figured out, the source of that might also be some good info to include in the CR per #41.
Comment #44
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPer last paragraph of #41 and #43.
Comment #45
alexpottRewrote the CR at https://www.drupal.org/node/2480761