Problem/Motivation

The patch being prepared in #2834889: Replace documentation and string references to drupal_render() was becoming unwieldy so it was decided that it should be split into smaller chunks.

Replace references to drupal_render() with \Drupal::service()... in sample code (*.api.php, @code/@endcode, etc.)

See the change record for details on code changes: https://www.drupal.org/node/2912696

Replace only sample code occurrences, and not all of them. Sample code will be between @code and @endcode tags.

Proposed resolution

Update references as appropriate.

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jo Fitzgerald created an issue. See original summary.

ryanhayes’s picture

Status: Active » Needs review
FileSize
50.47 KB

Did a cheeky find and replace.

benjifisher’s picture

Issue tags: +Nashville2018, +Novice
runeasgar’s picture

I am participating in the DrupalCon Nashville 2018 sprint, and will review this issue.

runeasgar’s picture

$ git apply -v drupal_render_replace-2938972-2.patch.txt

succeeded, with the exception of this one error:

error: core/modules/image/src/Tests/ImageDimensionsTest.php: No such file or directory

I assume this is because that file existed (and had drupal_render in it) when the patch was created, but that file no longer exists, or is in a different location. Searching for it. It's now here:

/core/modules/image/tests/src/Functional/ImageDimensionsTest.php

Looking at the patch file to understand the find/replace, so I can apply it to this file.

I will then look for a mentor to help me understand how I search the set of locations specified in the summary (*.api.php, @code/@endcode, etc.), to ensure all instances of drupal_render in these locations has been replaced.

jofitz’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

@runeasgar When git apply fails I tend to use patch -p1 instead, i.e.:
curl https://www.drupal.org/files/issues/drupal_render_replace-2938972-2.patch | patch -p1

Here's a blog article I wrote a while ago that you may find useful:
https://www.computerminds.co.uk/drupal-code/commands-i-use-when-creating...

I have set the status to Needs Work and added the Needs Reroll tag for someone (you, perhaps?) to re-roll the patch to apply to the latest codebase.

runeasgar’s picture

Status: Needs work » Needs review
FileSize
51.21 KB

Ok.. my first attempt at a reroll. I fixed the patch to apply to the new location for ImageDimensionsTest.php.

I am unsure how to "review" the patch to ensure that drupal_render has been replaced in all the appropriate places. I believe I need to find a set of commands (grep, I presume) that will let me search core *.api.php files and comments. I will look for a mentor to help me structure a command like this.. or maybe find something on Google!

Edit: I ended up running a command to remove my .orig files from the patch command, and inadvertently removed some other .orig files elsewhere in Drupal core (git -R rm core/*.orig). I left that in the patch. Not sure how things like that are handled.

runeasgar’s picture

Thank you, Jo - I did end up using patch for this. I didn't realize git apply was failing. Patch prompted me to locate the new file.

runeasgar’s picture

Regarding "review", I will say that looking at the actual patch file itself, it looks like the find/replace executed without creating any anamolies.

However, is replacing drupal_render() with \Drupal::service() the correct action? These functions do not seem to serve the same purpose. Do we need something more verbose, like \Drupal::service('renderer')->render() ?

If so, I could try to recreate the original find/replace and make this change, instead.

runeasgar’s picture

It's worth noting that changing these from drupal_render() to \Drupal::service() will make it much more manual (I think?) to change them to something more verbose later, since I presume \Drupal::service() is used in a lot of places. i.e., a find and replace won't work next time we wanted to change this? Or it would be a lot more obtuse to do so..

Mile23’s picture

Issue summary: View changes

Thanks, @runeasgar.

I've updated the issue summary to be a little more clear.

The change record we're trying to document here is: https://www.drupal.org/node/2912696

Basically change drupal_render() to \Drupal::service('renderer')->renderer().

Doing a search/replace will no doubt lead to incorrect word wrap, so a lot of it will have to be manually edited.

Status: Needs review » Needs work

The last submitted patch, 7: drupal_render_replace-2938972-6.patch, failed testing. View results

runeasgar’s picture

Ok, I don't understand the patch failure (looks related to those 2 .orig files I accidentally nuked).. but I don't think it matters, anyway, since the patch really just needs to be recreated from scratch.

@Mile23, I'm guessing "incorrect word wrap" refers to the problems the new, long, and unbroken string could create.. probably something related to coding guidelines (line length?) that I'm unaware of. Some guidance would be appreciated. I'm happy to do the work, so long as I know I can do it correctly the first time, since it's going to involve going through and hand-editing a lot of files (50, based on the original patch).

I will table this issue for now and look for another, while I await guidance.

MerryHamster’s picture

I added changes to a few places sample code occurrences which I have found

MerryHamster’s picture

Status: Needs work » Needs review
msankhala’s picture

Status: Needs review » Reviewed & tested by the community

I can verify that patch #14 applies cleanly and replaces drupal_render with Drupal::service() as per issue description.

Replace references to drupal_render() with \Drupal::service()... in sample code (*.api.php, @code/@endcode, etc.)

I also verified that drupal_render() inside @code and @endcode block was only available only for these 3 files. I verified this by doing a regex search inside phpstrom with filter = In Comments and regex = @code(?:[\s\S]*(drupal_render)[\s\S]*)@endcode.

This regex searches for any drupal_render in between @code and @endcode block.

msankhala’s picture

Here is screenshot of phpstrom search result with regex.
drupal_render inside @code block

savkaviktor16@gmail.com’s picture

Issue tags: -Needs reroll
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

All instances of drupal_render() in code blocks are fixed.

Committed and pushed fce8c26212 to 8.6.x and 66d0469e94 to 8.5.x. Thanks!

As docs backported to 8.5.x

  • alexpott committed fce8c26 on 8.6.x
    Issue #2938972 by runeasgar, MerryHamster, ryanhayes, msankhala, Jo...

  • alexpott committed 66d0469 on 8.5.x
    Issue #2938972 by runeasgar, MerryHamster, ryanhayes, msankhala, Jo...

Status: Fixed » Closed (fixed)

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