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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

justAChris created an issue. See original summary.

justAChris’s picture

This 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.

David_Rothstein’s picture

Title: Replace remaining !placeholder with :placeholder for URLs » Replace !placeholder and @placeholder with :placeholder for URLs

I 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.)

David_Rothstein’s picture

Title: Replace !placeholder and @placeholder with :placeholder for URLs » Replace remaining !placeholder and @placeholder with :placeholder for URLs
stefan.r’s picture

Status: Postponed » Active

Agreed 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.

kgoel’s picture

Status: Active » Needs review
FileSize
8.39 KB

I 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.

Status: Needs review » Needs work

The last submitted patch, 6: 2570355-6.patch, failed testing.

The last submitted patch, 6: 2570355-6.patch, failed testing.

kgoel’s picture

Status: Needs work » Needs review
FileSize
131.65 KB

It is late in the night. I might have missed some placeholder but posting what I have on my local.

Status: Needs review » Needs work

The last submitted patch, 9: 2570355-8.patch, failed testing.

The last submitted patch, 9: 2570355-8.patch, failed testing.

justAChris’s picture

If 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:

+++ b/core/modules/views_ui/views_ui.module
@@ -20,15 +20,15 @@ function views_ui_help($route_name, RouteMatchInterface $route_match) {
-      $output .= '<p>' . t('The Views UI module provides an interface for managing views for the <a href="@views">Views module</a>. For more information, see the <a href="@handbook">online documentation for the Views UI module</a>.', array('@views' => \Drupal::url('help.page', array('name' => 'views')), '@handbook' => 'https://www.drupal.org/documentation/modules/views_ui')) . '</p>';
+      $output .= '<p>' . t('The Views UI module provides an interface for managing views for the <a href=":views">Views module</a>. For more information, see the <a href=":handbook">online documentation for the Views UI module</a>.', array(':views' => \Drupal::url('help.page', array('name' => 'views')), ':handbook' => 'https://www.drupal.org/documentation/modules/views_ui')) . '</p>';
dawehner’s picture

Yeah the patch seems to be a little bit messed up partially

+++ b/core/modules/entity_reference/src/Tests/EntityReferenceAdminTest.php
@@ -170,7 +170,7 @@ public function testFieldAdminHandler() {
     $this->drupalPostAjaxForm($bundle_path . '/fields/' . $field_name, $edit, 'settings[handler]');
-    $this->assertRaw(t('No eligible views were found. <a href="@create">Create a view</a> with an <em>Entity Reference</em> display, or add such a display to an <a href="@existing">existing view</a>.', array(
+    $this->assertRaw(t('No eligible views were found. <a href=":create">Create a view</a> with an <em>Entity Reference</em> display, or add such a display to an <a href="@existing">existing view</a>.', array(
       ':create' => \Drupal::url('views_ui.add'),
       ':existing' => \Drupal::url('entity.view.collection'),
     )));

This for example uses not :existing

Sutharsan’s picture

Assigned: Unassigned » Sutharsan

Working on a new patch. Considering #9 doesn't apply and previous comments, I may start from scratch.

kgoel’s picture

This 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".

Sutharsan’s picture

Thanks, for the script. I think I prefer the manual replacement, but at least I can use it to double check the result.

Sutharsan’s picture

A new patch that includes the remaining href="! and all href="@ 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).

Status: Needs review » Needs work

The last submitted patch, 17: 2570355-17.patch, failed testing.

josephdpurcell’s picture

Assigned: Unassigned » josephdpurcell

The last submitted patch, 17: 2570355-17.patch, failed testing.

josephdpurcell’s picture

Status: Needs work » Needs review
FileSize
167.89 KB
1.07 KB

Fixes syntax error.

josephdpurcell’s picture

Assigned: josephdpurcell » Unassigned

I 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.

pfrenssen’s picture

Assigned: Unassigned » pfrenssen

Going to review this mammoth.

pwolanin’s picture

  1. +++ b/core/includes/install.core.inc
    @@ -1867,7 +1867,7 @@ function install_check_translations($langcode, $server_pattern) {
    +      'description' => t('The installer requires that you create a translations directory as part of the installation process. Create the directory %translations_directory . More details about installing Drupal are available in <a href=":install_txt">INSTALL.txt</a>.', array('%translations_directory' => $translations_directory, ':install_txt' => base_path() . 'core/INSTALL.txt')),
    

    Please convert this to using a Url object like Url::fromUri('base:core/INSTALL.txt')

  2. +++ b/core/includes/install.core.inc
    @@ -1907,7 +1907,7 @@ function install_check_translations($langcode, $server_pattern) {
    +      'description' => t('The installer requires to contact the translation server to download a translation file. Check your internet connection and verify that your website can reach the translation server at <a href=":server_url">!server_url</a>.', array(':server_url' => $server_url, '!server_url' => $server_url)),
    

    can you convert the other !server to @server?

  3. +++ b/core/includes/install.core.inc
    @@ -2106,12 +2106,12 @@ function install_check_requirements($install_state) {
    +              ':install_txt' => base_path() . 'core/INSTALL.txt',
    

    Again,

    Url::fromUri('base:core/INSTALL.txt')

  4. +++ b/core/lib/Drupal/Core/Installer/Exception/AlreadyInstalledException.php
    @@ -26,11 +26,11 @@ public function __construct(TranslationInterface $string_translation) {
    +      ':update-url' => $GLOBALS['base_path'] . 'update.php',
    

    Would be nice if we could kill the $GLOBALS here

dawehner’s picture

Status: Needs review » Needs work

.

stefan.r’s picture

@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 well

Opened #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?

pwolanin’s picture

This fixes for globals or base_path() should be trivial here - just take the 10 minutes to do it.

stefan.r’s picture

@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.

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
  1. +++ b/core/includes/install.core.inc
    @@ -1907,7 +1907,7 @@ function install_check_translations($langcode, $server_pattern) {
    -      'description' => t('The installer requires to contact the translation server to download a translation file. Check your internet connection and verify that your website can reach the translation server at <a href="!server_url">!server_url</a>.', array('!server_url' => $server_url)),
    +      'description' => t('The installer requires to contact the translation server to download a translation file. Check your internet connection and verify that your website can reach the translation server at <a href=":server_url">!server_url</a>.', array(':server_url' => $server_url, '!server_url' => $server_url)),
    

    Use '@' here to do the replacement, we're going to remove the '!' placeholder.

  2. +++ b/core/modules/responsive_image/src/ResponsiveImageStyleForm.php
    @@ -118,7 +118,7 @@ public function form(array $form, FormStateInterface $form_state) {
    -          $description = $this->t('See the <a href="!responsive_image_help">Responsive Image help page</a> for information on the sizes attribute.', array('!responsive_image_help' => (\Drupal::url('help.page', array('name' => 'responsive_image')))));
    +          $description = $this->t('See the <a href=":responsive_image_help">Responsive Image help page</a> for information on the sizes attribute.', array(':responsive_image_help' => (\Drupal::url('help.page', array('name' => 'responsive_image')))));
    

    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.

  3. +++ b/core/modules/system/src/Plugin/Block/SystemBrandingBlock.php
    @@ -106,8 +106,8 @@ public function blockForm($form, FormStateInterface $form_state) {
    -      $site_name_description = $this->t('Defined on the <a href="@information">Site Information</a> page.', array('@information' => $site_information_url));
    -      $site_slogan_description = $this->t('Defined on the <a href="@information">Site Information</a> page.', array('@information' => $site_information_url));
    +      $site_name_description = $this->t('Defined on the <a href=":information">Site Information</a> page.', array(':information' => $site_information_url));
    +      $site_slogan_description = $this->t('Defined on the <a href=":information">Site Information</a> page.', array(':information' => $site_information_url));
    

    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.

  4. +++ b/core/modules/system/system.install
    @@ -430,11 +430,12 @@ function system_requirements($phase) {
    +        $url = 'https://www.drupal.org/SA-CORE-2013-003';
             $requirements[$htaccess_file] = array(
               'title' => $info['title'],
               'value' => t('Not fully protected'),
               'severity' => REQUIREMENT_ERROR,
    -          'description' => t('See <a href="@url">@url</a> for information about the recommended .htaccess file which should be added to the %directory directory to help protect against arbitrary code execution.', array('@url' => 'https://www.drupal.org/SA-CORE-2013-003', '%directory' => $info['directory'])),
    +          'description' => t('See <a href=":url">@url</a> for information about the recommended .htaccess file which should be added to the %directory directory to help protect against arbitrary code execution.', array(':url' => $url, '@url' => $url, '%directory' => $info['directory'])),
    

    Good approach solving the double usage of @url.

dawehner’s picture

more at some other time.

  1. +++ b/core/includes/form.inc
    @@ -844,7 +844,7 @@ function batch_process($redirect = NULL, Url $url = NULL, $redirect_callback = N
    -      $batch['error_message'] = t('Please continue to <a href="@error_url">the error page</a>', array('@error_url' => $error_url->toString(TRUE)->getGeneratedUrl()));
    +      $batch['error_message'] = t('Please continue to <a href=":error_url">the error page</a>', array(':error_url' => $error_url->toString(TRUE)->getGeneratedUrl()));
    

    getGeneratedUrl() is not needed anymore

  2. +++ b/core/includes/install.core.inc
    @@ -1907,7 +1907,7 @@ function install_check_translations($langcode, $server_pattern) {
           'severity'    => REQUIREMENT_ERROR,
    -      'description' => t('The installer requires to contact the translation server to download a translation file. Check your internet connection and verify that your website can reach the translation server at <a href="!server_url">!server_url</a>.', array('!server_url' => $server_url)),
    +      'description' => t('The installer requires to contact the translation server to download a translation file. Check your internet connection and verify that your website can reach the translation server at <a href=":server_url">!server_url</a>.', array(':server_url' => $server_url, '!server_url' => $server_url)),
         );
    

    ! placeholder

alexpott’s picture

Looking 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.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
2.87 KB
167.96 KB
stefan.r’s picture

The last submitted patch, 32: 2570355-31.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 33: 2570355-32.patch, failed testing.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
730 bytes
167.96 KB
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Re #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.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Reviewed 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:

$build['#suffix'] = t('Check the messages and <a href="@:url">try again<
/a>.', array('@:url' => UrlHelper::stripDangerousProtocols(drupal_requirements_url($severity))));

We added :placeholder with support for stripDangerousProtocols so let's not do it twice.

array('@:url' => $url, 'ht
tps://www.drupal.org/SA-CORE-2013-003@url' => $url

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.

 '#markup' => t('To run cron from outside the site, go to <a href="@cron:url">@cron</a>', ['@cron' => \Drupal:::url('system.cron', ['key' => \Drupal::state()->get('system.cron_key')$cron_url,[ 'absolute@cron' => TRUE]])$cron_url]),

Same question here.

Everything else looks very straightforward.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
168.13 KB
167.78 KB

Did color words too:) scroll blindness.

Re #38

  1. Done, thanks for finding this @catch
  2. I'd prefer the placeholders because the URL change really shouldn't break translation.
  3. Same
stefan.r’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.9 KB

The 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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 39: replace_remaining-2570355-39.patch, failed testing.

alexpott’s picture

Status: Needs work » Fixed

Committed 6bfdc3b and pushed to 8.0.x. Thanks!

  • alexpott committed 6bfdc3b on 8.0.x
    Issue #2570355 by Sutharsan, stefan.r, kgoel, josephdpurcell, joelpittet...
alexpott’s picture

The failure #41 was just me cancelling pift.

The last submitted patch, 32: 2570355-31.patch, failed testing.

The last submitted patch, 33: 2570355-32.patch, failed testing.

The last submitted patch, 36: 2570355-36.patch, failed testing.

Status: Fixed » Closed (fixed)

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

The last submitted patch, 21: 2570355-20.patch, failed testing.