[meta] Replace all calls of the deprecated Drupal::url() from all the modules of Drupal Core

Working on it!!!!

Unassigning to let other people work on the issue - 3 days have passed

Hi!

I have found 387 occurrences in code and I'm going to work on it now.

I have changed all occurrences that I found.

sorry, double posted by mistake.

Hi! I have tried to correct errors from last patch.

working on this issue

I found 492 hits and will replacing it

Straight reroll. Rebase + minor merge in dblog.module.

Still 10 usages. See attached.

OK, just saw there was a parent issue on this. Per that, it looks like there should be three issues. Which one should we scope this as?

When applying the patch I get:

$ git apply ../2869074-22.patch
error: patch failed: core/modules/block_content/block_content.module:15
error: core/modules/block_content/block_content.module: patch does not apply

Because of this, I'm adding the "needs reroll" tag and setting the status to "Needs work".

After applying the changes with the patch utility, there are 17 occurrences of Drupal::url( including those from the failled hunks.

Rerolled patch

Fix for failing patch.

Improved the coding standard errors

The patch doesn't apply anymore.

I will do a re-roll.

re-rolled the patch

Adding a patch for the deprecated code.

Patch was failing again adding the patch.

@vakulrai, can you please also post an interdiff with each patch you post? It is difficult to tell otherwise why and how the patch in #43 decreased in size from the one in #41 by about 14 KB. Thanks.

Just improve minor CS issue.

+++ b/core/modules/path/path.module
@@ -10,9 +10,11 @@
+
 /**
  * Implements hook_help().
  */
+
 function path_help($route_name, RouteMatchInterface $route_match) {

There shouldn't be a blank line between the docblock and the function definition.

mpdonadio i have removed the blank line

Thank you

Attached patch replaces old URL calls everywhere except from core/modules/rdf/rdf.module which when patched break the users administration page (and possibly others).

Submitting for review and testing.

Rerolling with phpcs fixes. Updated #2868889: [Meta] Replace all calls of the deprecated Drupal::url() from the code base too.

Comment in #53 is still valid: rdf module is not updated.

Rerolling, as phpcs introduced an error with duplicate lines.

More PHPCS corrections.

Credits to be credited: bserem, kostask, Sutharsan, sysosmaster

A new patching coming after the social event in DrupalEurope.

Rerolling

@bserem: please leave the "credits" section for the maintainers to adjust, it's improper / impolite to remove people from this list if you aren't in a position to commit changes.

@Damien: if you check my comment you will see that I removed no one. I simply typed down the usernames of the people that jumped in to help with the issue in DrupalEurope. Thats why I phrased it "credits to be credited", so that I do not forget who they were.

For that reason, I would kindly ask you to rephrase your last comment.

Rerolling after fixing linting errors.

@bserem: My apologies, someone has been changing credits on issues and I mistakenly thought you might have inadvertently done so while noting who worked on the issue with you. I'm sincerely sorry for my mistake.

Could whomever has been changing the commit credits list on this issue please stop doing so.

@bserem, Damien wasn't accusing you of malice. We have just started noticing issues where older credits are vanishing. I restored them, and added the ones you listed (and for full transparency, I had credit vanish).

Also please make sure you post an interdiff with each and every patch. It's one of the few ways a 300k+ patch can be reviewed properly.

Thanks.

OK, here should be a better starting point. #38 looked like the last patch that actually ran.

$ git checkout 8.7.x
$ git log --before '31 Jan 2018' # find a good hash to patch against
$ git checkout d4e8416189983b2ab920eff7b31ffc7a0359ddb3
$ git checkout -b 2869074-38
$ git apply 2869074-38.patch
$ git add -A
$ git commit -m 2869074-38.patch
$ git rebase 8.7.x

There were 12 merge conflicts, I chose HEAD in every case.

$ git add -A
$ git rebase --continue
$ git branch -m 2869074-80
$ git diff 8.7.x > 2869074-80.patch

Think this will run.

And these look like the remaining usages

$ grep -R '\Drupal::url(' core

core/includes/theme.inc:  $variables['front_page'] = \Drupal::url('');
core/modules/contact/tests/src/Functional/ContactSitewideTest.php:      ':href' => \Drupal::url('entity.contact_form.edit_form', ['contact_form' => 'personal']),
core/modules/dblog/dblog.module:    '#description' => t('The maximum number of messages to keep in the database log. Requires a cron maintenance task.', [':cron' => \Drupal::url('system.status')]),
core/modules/history/history.module:      $output .= '
' . t('The History module keeps track of which content a user has read. It marks content as new or updated depending on the last time the user viewed it. History records that are older than one month are removed during cron, which means that content older than one month is always considered read. The History module does not have a user interface but it provides a filter to Views to show new or updated content. For more information, see the online documentation for the History module.', [':views-help' => (\Drupal::moduleHandler()->moduleExists('views')) ? \Drupal::url('help.page', ['name' => 'views']) : '#', ':url' => 'https://www.drupal.org/documentation/modules/history']) . '
';
core/modules/image/tests/src/Functional/ImageFieldDisplayTest.php:    $this->assertLinkByHref(\Drupal::url('entity.image_style.collection'), 0, 'Link to image styles configuration is found');
core/modules/image/tests/src/Functional/ImageFieldDisplayTest.php:    $this->assertNoLinkByHref(\Drupal::url('entity.image_style.collection'), 'Link to image styles configuration is absent when permissions are insufficient');
core/modules/search/tests/src/Functional/SearchConfigSettingsFormTest.php:    $this->assertIdentical($elements[0]->getAttribute('href'), \Drupal::url('search.view_node_search'));
core/modules/search/tests/src/Functional/SearchConfigSettingsFormTest.php:    $this->assertIdentical($elements[1]->getAttribute('href'), \Drupal::url('search.view_dummy_search_type'));
core/modules/search/tests/src/Functional/SearchConfigSettingsFormTest.php:    $this->assertIdentical($elements[2]->getAttribute('href'), \Drupal::url('search.view_user_search'));
core/modules/search/tests/src/Functional/SearchLanguageTest.php:    $this->assertUrl(\Drupal::url('search.view_node_search', [], ['query' => ['keys' => ''], 'absolute' => TRUE]), [], 'Correct page redirection, no language filtering.');

That's what I get for not checking my file mask when searching for the actual conflicts.

Ok, let's see if this at least starts to show fails instead of just "build successful". I think the

-use Drupal\core\Url;
+use Drupal\Core\Url;

should fix a bunch of the problems (which I did a global search/replace on).

Otherwise, I think the

@@ -102,7 +102,7 @@ public function getFeedEditArray($feed_url = NULL, array $edit = []) {
       $feed_url = URL::fromRoute('view.frontpage.feed_1', [], [
         'query' => ['feed' => $feed_name],
         'absolute' => TRUE,
-      ]);
+      ])->toString();
     }
     $edit += [
       'title[0][value]' => $feed_name,

are going to be where a lot of the problems are going to be. If you still get the "build successful", click on the "build successful" link and then "view result on dispatcher". Start one by one, and run them locally and see what is going on. Every time you see a "Exception: Warning: strpos() expects parameter 1 to be string, object given", you need a ->toString() somewhere on one of the conversions.

These two things got AddFeedTest passing. Use that as a starting point.

For now, ignore lint and coding standard errors. Just try to reduce the number of fails, and make sure you add an interdiff with each patch.

For what it is worth, I do the branch-per-patch method and name the branches "2869074-80", "2869074-82", "2869074-85", etc. Then I use

#!/bin/bash

ISSUE=`git rev-parse --abbrev-ref HEAD | cut -d'-' -f1`
COMMENT1=`git branch | grep -B 1 '*' | head -1 | cut -d'-' -f2`
COMMENT2=`git rev-parse --abbrev-ref HEAD | cut -d'-' -f2`

git diff origin/8.7.x > `git rev-parse --abbrev-ref HEAD`.patch

if [ "${COMMENT1}" != "${COMMENT2}" ]; then
    git diff ${ISSUE}-${COMMENT1} > interdiff-${COMMENT1}-${COMMENT2}.txt
fi

to make my patch and interdiff at the same time, after I have committed everything.

Removing redundant DrupalEurope2018.

@mpdonadio:
Thanks for the oneliner for interdiffs! It will help speed things up.
I will work on this again on Friday, in the sprints, because now the conference is running and there a lot of things happening here.

Builds are succesfull and drupal is running just fine, but we need to add '->toString()' to a LOT of lines. I believe massively changing this in all of core module is difficult and time consuming, because it is easy for conflicts to be added while other people work on other issues.
Nontheless, I have this on my list, and it is a nice timing now that 8.6 just got released.

Our initial idea here at the conference was to script this, so that we could re-roll patches if needed again and again easily.
However, scripting it requires some very tricky regex, and we haven't managed it yet.

@mradcliffe I remember we were adding such tags when things were being worked in DrupalCon sprints.

@DamienMcKenna no harm done ;)

Yeah, mega patches are tough. You can do a lot with scripting / creative regex search/replace, but often you just need to just go in an manually touch and debug this stuff...

This patch has revealed the difficulties of doing it all in one swoop. I would suggest rescoping this into four issues/patches:

1. Replace all instances in procedural code (*.inc and *.module).
2. Replace all instances in classes (*.php, but not *Test.php).
3. Replace all instances in tests (*Test.php).
4. Replace anything that got missed was really hard.

I think that will help the patch move faster. I actually don't think there will be much conflict with other work. The changes are rather small, it and should work will with `git patch apply` as long as there is frequent rebasing against HEAD along the way.

I discussed this issue with @bserem and @kostask. Instead of splitting the patch up by file location or even type of file, it's better to split it up by the kind of replacement. Reference: https://www.drupal.org/core/scope#files (and following)

We scanned through the patch and identified several specific patterns:

1. hook_help() implementations
2. URL assertions in tests (the test runner may already have the URL generation service, so we would not want to use the static method if it is already available as a service)
3. Other raw string assertions in tests (as above; some of these might also just be not-exactly-testing-the-right-thing)
4. *.api.php documentation
5. In-code documentation (there was at least one docblock, but this would apply to other docblocks and potentially inline docs as well)
6. Redirect responses (there were at least two)
7. Form actions (there was one in an AJAX form fixture)
8. Theme template variables (one for the Seven theme)

Additionally, there are two specific modules I recommend evaluating in their own issue: Views and the Update module. Some of those modules' usages will fall under the above categories, but the remainder should be examined on a case-by-case basis since both modules could be doing special things with the URLs. For the update module, at least some of them might fall into a more general pattern of "URLs in status warnings" or such.

Finally, a separate consideration is whether the existing Drupal::url() calls are single-line or multi-line, because that affects the replacement script for the larger changes that we'll need to script. So we might want to handle the multiline calls separately depending on the scope of those larger patches. At the least, we should document how many existing calls are multi-line out of the total number.

Thanks!

Since this was the only child of #2868889: [Meta] Replace all calls of the deprecated Drupal::url() from the code base but is the issue that actually has a patch and some discussion, let's actually use this one as the meta. That way we can also make sure contributors from this issue get credited on those. I'm going to close the old parent as a duplicate.

@kostask mentioned that they plan to open the child issues next.

Thanks everyone!

Please follow this patch

Thank you for providing an additional patch, @kalyansamanta. However this issue is now a "meta" issue based on the new Category "Plan", which means we're not actually posting the patches to do any work here.

As @xjm notes in #90, based on the work and discussion at DrupalEurope by her, @bserem and @kostask, the next step is to create follow-up tasks based on #91 which turned this issue into a meta issue.

As well, I think the issue summary could note that this is now itself a meta based on #90 and #91. I gave that a start, but I think what's remaining is to add Problem/Motivation and Proposed Resolution sections to this issue summary based on the issue summary template so that those who work on the child issues can refer back to how to do it.

Tagging for DrupalNorth 2019

Tagging for DrupalCamp Colorado 2019 (Sunday August 4)

Tagging for badcamp2019, thanks! (October 2-5)

Hi I have added the latest patch to run the test on 8.8.x branch. Thanks

I think there's nothing left here, \Drupal::url() has been fully deprecated a while ago, this is a leftover.