Problem/Motivation
Follow-up to #2565895: Add a new :placeholder to SafeMarkup::format() for URLs that handles bad protocols
Replace usage of !placeholder with :placeholder for URL HTML outputs.
Proposed resolution
Replace usage of !placeholder in for URL HTML outputs.
The following issue already exists, so should be excluded from here: #2560783: Replace !placeholder with :placeholder for URLs in hook_help() implementations
To determine scope:
Start with the following bash command
egrep -r '\Wt\(.*\!([a-zA-Z])+' * | grep 'href="!' | grep -v "vendor" | grep -v "\.js" | grep -v "\.css" | grep -v "~"
Remaining tasks
Discuss plan of action, segmenting replacement into smaller pieces
User interface changes
Comment | File | Size | Author |
---|---|---|---|
#40 | interdiff-36-39.txt | 2.9 KB | stefan.r |
#39 | replace_remaining-2570355-39.patch | 167.78 KB | joelpittet |
#39 | interdiff.txt | 168.13 KB | joelpittet |
#36 | 2570355-36.patch | 167.96 KB | stefan.r |
#36 | interdiff-32-36.txt | 730 bytes | stefan.r |
Comments
Comment #2
justAChris CreditAttribution: justAChris as a volunteer commentedThis is postoned on the completion of #2565895: Add a new :placeholder to SafeMarkup::format() for URLs that handles bad protocols to determine scope.
Bash script could use a bit of updating.
Comment #3
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI think this needs to cover converting @placeholder also. (Certainly feel free to split that off to a separate issue if it makes sense implementation-wise, but it seems equally critical to me.)
Comment #4
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedComment #5
stefan.r CreditAttribution: stefan.r commentedAgreed with converting the @placeholders as well.
We can already work on this if we do a combined patch with #2565895: Add a new :placeholder to SafeMarkup::format() for URLs that handles bad protocols which seems close to RTBC now. @kgoel will have a look at this.
Comment #6
kgoel CreditAttribution: kgoel at Forum One commentedI am sure there are more instances of "@placeholder" which needs to be replaced with ":placeholder" but I need to figure out easy way instead of doing it all manually.
Comment #9
kgoel CreditAttribution: kgoel at Forum One commentedIt is late in the night. I might have missed some placeholder but posting what I have on my local.
Comment #12
justAChris CreditAttribution: justAChris as a volunteer commentedIf we've determined that #2560783: Replace !placeholder with :placeholder for URLs in hook_help() implementations is a reasonable way to split off !placeholder instances in hook_help(), then either all @placeholder in hook_help() should be added there or a similar issue should be filed.
Example, more similar in patch:
Comment #13
dawehnerYeah the patch seems to be a little bit messed up partially
This for example uses not :existing
Comment #14
Sutharsan CreditAttribution: Sutharsan as a volunteer commentedWorking on a new patch. Considering #9 doesn't apply and previous comments, I may start from scratch.
Comment #15
kgoel CreditAttribution: kgoel at Forum One commentedThis is what I used to find and replace placeholders.
find . -type f -name '*.module' -exec sed -i '' "s/ href=\"@/ href=\":/" {} +
find . -type f -name '*.php' -exec sed -i '' "s/ href=\"@/ href=\":/" {} +
It might save you some time so you don't have to change placeholders in href tag since it takes care of that but I didn't find an easy way to change URL placeholders and I had to manual look for URL placeholders in patch file and replace with ":placeholder".
Comment #16
Sutharsan CreditAttribution: Sutharsan as a volunteer commentedThanks, for the script. I think I prefer the manual replacement, but at least I can use it to double check the result.
Comment #17
Sutharsan CreditAttribution: Sutharsan as a volunteer commentedA new patch that includes the remaining
href="!
and allhref="@
I could find.I included some separate patches (marked -do-not-test) in case someone decides to split this issue. It has become quite a bit patch (again).
I did not build on top of the #9 patch because it did not apply and it contained file changes which are out of scope (mostly missing empty line at end of file and changes to vendor files). It looked like the patch was edited after it was generated to change the !/@ in the array keys to :
When applied on top of #2560783-84: Replace !placeholder with :placeholder for URLs in hook_help() implementations, I could not find any remaining
href=['|"][!|@]
(checked using PhpStorm search).Comment #19
josephdpurcell CreditAttribution: josephdpurcell commentedComment #21
josephdpurcell CreditAttribution: josephdpurcell commentedFixes syntax error.
Comment #22
josephdpurcell CreditAttribution: josephdpurcell commentedI can confirm that after applying patch #21 AND 2560783-83.patch, the following command finds no files, which is what we want:
egrep -r '\Wt\(.*\!([a-zA-Z])+' * | grep 'href="!' | grep -v "vendor" | grep -v "\.js" | grep -v "\.css" | grep -v "~"
I did not do a review of the actual changes; I'll circle back to review this in a bit if no one else takes it.
Comment #23
pfrenssenGoing to review this mammoth.
Comment #24
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedPlease convert this to using a Url object like Url::fromUri('base:core/INSTALL.txt')
can you convert the other !server to @server?
Again,
Url::fromUri('base:core/INSTALL.txt')
Would be nice if we could kill the $GLOBALS here
Comment #25
dawehner.
Comment #26
stefan.r CreditAttribution: stefan.r commented@pwolanin I think
<a href=":placeholder">@placeholder</a>
makes more sense than<a href=":placeholder">:placeholder</a>
as well, which we do in #2560783: Replace !placeholder with :placeholder for URLs in hook_help() implementations - so let's fix it there as wellOpened #2572133: Don't use base_path() in URL placeholder values for the other 3 points, if we go down the route of fixing all the placeholder values we might hold up these issues for too long?
Comment #27
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedThis fixes for globals or base_path() should be trivial here - just take the 10 minutes to do it.
Comment #28
stefan.r CreditAttribution: stefan.r commented@pwolanin OK, but in terms of fixing the placeholder values let's stick to fixing just that, as this came up in the other issue as well and core does a lot more questionable things there than just using base_path()
Will wait for @prfrenssen's review and roll a patch.
Comment #29
pfrenssenUse '@' here to do the replacement, we're going to remove the '!' placeholder.
The parentheses around the
\Drupal::url()
call are not needed. This is not strictly in scope but if we are touching this line we can just as well fix it.This is not strictly in scope for this issue, but the replacement array is identical for both lines. Now that we are touching this we can factor this out in a separate variable.
Good approach solving the double usage of @url.
Comment #30
dawehnermore at some other time.
getGeneratedUrl() is not needed anymore
! placeholder
Comment #31
alexpottLooking at #24 I think we should keep the conversions as simple as possible - way less risky - we've had all sorts of fails with url creation during installer lets not risk any here.
Comment #32
stefan.r CreditAttribution: stefan.r commentedComment #33
stefan.r CreditAttribution: stefan.r as a volunteer commentedComment #36
stefan.r CreditAttribution: stefan.r as a volunteer commentedComment #37
joelpittetRe #24
#24.1 Out of scope
#24.2 Done
#24.3 Out of scope
#24.4 Out of scope
Re #29
1) Done
2) Done
3) Out of scope
4) OK
#30
1) Out of scope
2) Done
Rather not put any risky changes in such a large patch, good to spot them and create follow-ups to fix them if they are really needed.
Comment #38
catchReviewed with diff --color-words.
Agreed with keeping this to a straight conversion and opening (major or normal) follow-ups for any changes that aren't in very strict scope here.
However:
We added :placeholder with support for stripDangerousProtocols so let's not do it twice.
The @url isn't in an href, but why not use :url here too anyway? Or at least, have we decided whether to or not for literal URL placeholders that aren't in an href? If the answer is 'we decided not to, this is correct' then that's fine, just checking.
Same question here.
Everything else looks very straightforward.
Comment #39
joelpittetDid color words too:) scroll blindness.
Re #38
Comment #40
stefan.r CreditAttribution: stefan.r as a volunteer commentedThe patch is green on CI and merely replaces a double call to stripDangerousProtocols, so back to RTBC.
Uploading an actual interdiff with #36 for reference.
Comment #42
alexpottCommitted 6bfdc3b and pushed to 8.0.x. Thanks!
Comment #44
alexpottThe failure #41 was just me cancelling pift.