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
Comment | File | Size | Author |
---|---|---|---|
#17 | drupal_render-code-block-search-result.png | 564.68 KB | msankhala |
#14 | 2938972-14.patch | 1.45 KB | MerryHamster |
Comments
Comment #2
ryanhayes CreditAttribution: ryanhayes as a volunteer commentedDid a cheeky find and replace.
Comment #3
benjifisherComment #4
runeasgar CreditAttribution: runeasgar as a volunteer commentedI am participating in the DrupalCon Nashville 2018 sprint, and will review this issue.
Comment #5
runeasgar CreditAttribution: runeasgar as a volunteer commented$ 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.
Comment #6
jofitz CreditAttribution: jofitz at ComputerMinds commented@runeasgar When
git apply
fails I tend to usepatch -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.
Comment #7
runeasgar CreditAttribution: runeasgar as a volunteer commentedOk.. 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.
Comment #8
runeasgar CreditAttribution: runeasgar as a volunteer commentedThank 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.
Comment #9
runeasgar CreditAttribution: runeasgar as a volunteer commentedRegarding "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.
Comment #10
runeasgar CreditAttribution: runeasgar as a volunteer commentedIt'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..Comment #11
Mile23Thanks, @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.
Comment #13
runeasgar CreditAttribution: runeasgar as a volunteer commentedOk, 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.
Comment #14
MerryHamster CreditAttribution: MerryHamster at Skilld commentedI added changes to a few places sample code occurrences which I have found
Comment #15
MerryHamster CreditAttribution: MerryHamster at Skilld commentedComment #16
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedI can verify that patch #14 applies cleanly and replaces drupal_render with Drupal::service() as per issue description.
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 withfilter = In Comments
andregex = @code(?:[\s\S]*(drupal_render)[\s\S]*)@endcode.
This regex searches for any
drupal_render
in between @code and @endcode block.Comment #17
msankhala CreditAttribution: msankhala as a volunteer and at Srijan | A Material+ Company commentedHere is screenshot of phpstrom search result with regex.
Comment #18
savkaviktor16@gmail.com CreditAttribution: savkaviktor16@gmail.com at Skilld commentedComment #19
alexpottAll 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