Replace drupal_render() in docblock and comments outside of @param, @return, @link, @see and outside of @code - @endcode

Shouldn't we change this issue title Replace drupal_render() in one-line summaries => Replace drupal_render() in docblock and comments outside of @param, @return, @link, @see and outside of @code - @endcode

because drupal_render is also mentioned in docblocks and comments which are not the one-line summary. Rest drupal_render() in dockblocks are already fixed in #2938970: Replace drupal_render() in @param, @return, @see, @link, etc. and #2938972: Replace drupal_render() in sample code

After fixing this issue drupal_render will only remain in actual code in function/method body, not in docblock or comments which can be fixed in #2938973: Replace drupal_render() within the Render component

Here is my first patch according to my comment #2. After this patch drupal_render() only remains in core/include/common.inc where it is actually referencing to drupal_render() function.

Here is updated patch.

reroll to 8.7.x, and added little fixes

@MerryHamster Thanks for working on this issue. I can confirm the patch #9 is fixing all the mention of drupal_render() as per issue title. After applying the patch the only places where drupal_render() is remaining is inside the actual render components or tests which should be fixed in #2938973: Replace drupal_render() within the Render component

Mention of drupal_render() inside the core/includes/common.inc is expected.

After applying the patch:

d8core 2938969-replace-drupal_render-outside* rg 'drupal_render\(\)' core
core/includes/common.inc
776: * links if drupal_render() is called on it, but calling drupal_render() on the
958: * The first time render() or drupal_render() is called on an element tree,
961: * render() or drupal_render() will not traverse the child tree of this element
987: * The first time render() or drupal_render() is called on an element tree,
990: * render() or drupal_render() will not traverse the child tree of this element

core/lib/Drupal/Core/Render/Renderer.php
555:        throw new \LogicException('A stray drupal_render() invocation with $is_root_call = TRUE is causing bubbling of attached assets to break.');

core/tests/Drupal/KernelTests/Core/Render/Element/RenderElementTypesTest.php
194:      $this->assertTrue($result, '"' . $element['name'] . '" input rendered correctly by drupal_render().');
225:      $this->assertTrue($result, '"' . $element['name'] . '" is rendered correctly by drupal_render().');
241:    $this->assertTrue($result, '"' . $element['name'] . '" is rendered correctly by drupal_render().');

core/modules/system/tests/src/Kernel/Theme/ThemeTest.php
138:    $this->assertThemeOutput('theme_test_render_element_children', $element, 'Foo', 'drupal_render() avoids #theme recursion loop when rendering a render element.');
146:    $this->assertThemeOutput('theme_test_render_element_children', $element, 'Foo', 'drupal_render() avoids #theme_wrappers recursion loop when rendering a render element.');

I am using ripgrep to search, you can verify the same with grep.

Crediting people from the parent issue.

+++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -1604,7 +1604,8 @@ function hook_entity_view_mode_alter(&$view_mode, Drupal\Core\Entity\EntityInter
- * Alter entity renderable values before cache checking in drupal_render().
+ * Alter entity renderable values before cache checking in
+ * \Drupal::service('renderer')->render().

@@ -1630,7 +1631,8 @@ function hook_ENTITY_TYPE_build_defaults_alter(array &$build, \Drupal\Core\Entit
- * Alter entity renderable values before cache checking in drupal_render().
+ * Alter entity renderable values before cache checking in
+ * \Drupal::service('renderer')->render().

+++ b/core/modules/node/node.module
@@ -816,7 +816,8 @@ function node_view(NodeInterface $node, $view_mode = 'full', $langcode = NULL) {
- * Constructs a drupal_render() style array from an array of loaded nodes.
+ * Constructs a \Drupal\Core\Render\RendererInterface::render() style array from
+ * an array of loaded nodes.

+++ b/core/modules/system/tests/modules/theme_test/theme_test.module
@@ -145,12 +145,13 @@ function template_preprocess_theme_test_render_element(&$variables) {
- * Theme function for testing rendering of child elements via drupal_render().
+ * Theme function for testing rendering of child elements via
+ * \Drupal::service('renderer')->render().

+++ b/core/modules/taxonomy/taxonomy.module
@@ -221,7 +221,8 @@ function taxonomy_term_view(Term $term, $view_mode = 'full', $langcode = NULL) {
- * Constructs a drupal_render() style array from an array of loaded terms.
+ * Constructs a \Drupal\Core\Render\RendererInterface::render() style array from
+ * an array of loaded terms.

+++ b/core/modules/toolbar/src/Element/Toolbar.php
@@ -50,7 +50,8 @@ public function getInfo() {
-   * Builds the Toolbar as a structured array ready for drupal_render().
+   * Builds the Toolbar as a structured array ready for
+   * \Drupal\Core\Render\RendererInterface::render().

+++ b/core/modules/user/user.module
@@ -907,7 +907,8 @@ function user_view($account, $view_mode = 'full', $langcode = NULL) {
- * Constructs a drupal_render() style array from an array of loaded users.
+ * Constructs a \Drupal\Core\Render\RendererInterface::render() style array from
+ * an array of loaded users.

+++ b/core/modules/views/src/Plugin/views/field/FieldHandlerInterface.php
@@ -255,7 +255,8 @@ public function renderText($alter);
-   * Passes values to drupal_render() using $this->themeFunctions() as #theme.
+   * Passes values to \Drupal::service('renderer')->render() using
+   * $this->themeFunctions() as #theme.
    *

+++ b/core/tests/Drupal/KernelTests/Core/Render/Element/RenderElementTypesTest.php
@@ -7,7 +7,8 @@
- * Tests the markup of core render element types passed to drupal_render().
+ * Tests the markup of core render element types passed to
+ * \Drupal::service('renderer')->render().

These need to be a one-liner to comply with coding standards.

i am working on it.

Pls find the Updated patch and interdiff.

+++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -1633,7 +1633,7 @@ function hook_entity_view_mode_alter(&$view_mode, Drupal\Core\Entity\EntityInter
- * Alter entity renderable values before cache checking in drupal_render().
+ * Alter entity renderable values before cache checking in \Drupal::service('renderer')->render().

@@ -1659,7 +1659,7 @@ function hook_ENTITY_TYPE_build_defaults_alter(array &$build, \Drupal\Core\Entit
- * Alter entity renderable values before cache checking in drupal_render().
+ * Alter entity renderable values before cache checking in \Drupal::service('renderer')->render().

+++ b/core/modules/node/node.module
@@ -817,7 +817,7 @@ function node_view(NodeInterface $node, $view_mode = 'full', $langcode = NULL) {
- * Constructs a drupal_render() style array from an array of loaded nodes.
+ * Constructs a \Drupal\Core\Render\RendererInterface::render() style array from an array of loaded nodes.

+++ b/core/modules/system/tests/modules/theme_test/theme_test.module
@@ -145,12 +145,12 @@ function template_preprocess_theme_test_render_element(&$variables) {
- * Theme function for testing rendering of child elements via drupal_render().
+ * Theme function for testing rendering of child elements via \Drupal::service('renderer')->render().

+++ b/core/modules/taxonomy/taxonomy.module
@@ -203,7 +203,7 @@ function taxonomy_term_view(Term $term, $view_mode = 'full', $langcode = NULL) {
- * Constructs a drupal_render() style array from an array of loaded terms.
+ * Constructs a \Drupal\Core\Render\RendererInterface::render() style array from an array of loaded terms.

+++ b/core/modules/toolbar/src/Element/Toolbar.php
@@ -50,7 +50,7 @@ public function getInfo() {
-   * Builds the Toolbar as a structured array ready for drupal_render().
+   * Builds the Toolbar as a structured array ready for \Drupal\Core\Render\RendererInterface::render().

+++ b/core/modules/user/user.module
@@ -913,7 +913,7 @@ function user_view($account, $view_mode = 'full', $langcode = NULL) {
+ * Constructs a \Drupal\Core\Render\RendererInterface::render() style array from an array of loaded users.

+++ b/core/modules/views/src/Plugin/views/field/FieldHandlerInterface.php
@@ -255,7 +255,7 @@ public function renderText($alter);
-   * Passes values to drupal_render() using $this->themeFunctions() as #theme.
+   * Passes values to \Drupal::service('renderer')->render() using $this->themeFunctions() as #theme.

+++ b/core/tests/Drupal/KernelTests/Core/Render/Element/RenderElementTypesTest.php
@@ -7,7 +7,7 @@
- * Tests the markup of core render element types passed to drupal_render().
+ * Tests the markup of core render element types passed to \Drupal::service('renderer')->render().

These have to be one line and less than 80 cols. It's tricky.

Also I'm not sure the interdiff is correct because it looks like it is adding new drupal_render() usages being introduced but there are no in the patch.

I don't know what gone wrong with interdiff file in comment #26. But patch file looks ok to me.

@alexpott, If u look at patch file then all changes are already included.

The patch in #26 did not address the comments in #24 or #27.

Here is a new patch re-rolled against HEAD which fixes all the things pointed out in all the previous comments. This patch also fixes six places that weren't touched by the patch in #26. After this patch there are no more references to drupal_render() in the codebase.

The interdiff is a bit messed up because the line numbers of some of the hunks have changed so much in the past three years that interdiff can't match up the hunks properly. Because the previous patch is so old, I suggest ignoring the interdiff and reviewing this from scratch.

Some feedback below.

#2794261: Deprecate render() function in common.inc is tangentially related, if anyone wants to review.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php
   @@ -17,11 +17,12 @@
   + * When controllers call \Drupal::service('renderer')->render()
   

   I think this should just refer to RendererInterface::render().

2. +++ b/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php
   @@ -36,8 +37,8 @@
   + * \Drupal::service('renderer')->render(), and bubbleable metadata was
   

   Same

3. +++ b/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php
   @@ -124,8 +125,8 @@ protected function wrapControllerExecutionInRenderContext($controller, array $ar
   +    // \Drupal::service('renderer')->render() outside of a render context, then
   

   Same

4. +++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
   @@ -245,10 +245,11 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state);
   +   * \Drupal::service('renderer')->render() (for rendering each element).
   

   And again, because that is where the relevant docs live.

5. +++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
   @@ -269,8 +270,8 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state);
   +   * self::doValidateForm() and \Drupal::service('renderer')->render(),
   

   Same.

6. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
   @@ -171,7 +171,8 @@ public function build(array $tree) {
          $build['#sorted'] = TRUE;
   

   #sorted is not checked by the renderer itself, should this refer to Element::children()?

7. +++ b/core/lib/Drupal/Core/Render/Element/Link.php
   @@ -111,9 +111,10 @@ public static function preRenderLink($element) {
   +   * links if \Drupal::service('renderer')->render() is called on it, but
   

   Again I think this should refer to the interface.

8. +++ b/core/modules/book/src/BookManager.php
   @@ -596,7 +596,8 @@ public function bookTreeOutput(array $tree) {
   +      // Make sure \Drupal::service('renderer')->render() does not re-order the
   

   Same as before, render() does not reorder the links anyway.

9. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityViewBuilderTest.php
   @@ -45,7 +45,8 @@ public function testEntityViewBuilderCache() {
   +    // Force a request via GET so we can get
   +    // \Drupal::service('renderer')->render() cache working.
   

   This could just say "render cache" I think?

10. +++ b/core/tests/Drupal/KernelTests/Core/Render/Element/RenderElementTypesTest.php
    @@ -182,7 +182,7 @@ public function testMoreLink() {
    +      $this->assertNotEmpty($result, '"' . $element['name'] . '" input rendered correctly by \Drupal::service(\'renderer\')->renderRoot().');
    

    I would just drop "by" and everything after it.

New patch that addresses #35.

Hi, patch 2938969-36.patch looks good.

Steps performed:
1. Got code from 9.3.x-dev.
2. Applied 2938969-36.patch.
3. Reviewed all changed files.
4. Checked PHPCS (Drupal and DrupalPractice standards) against all changed files. No issues have been introduced.
5. Tested classes which had changes in assert methods. Tests passed (images attached).

Thanks for reviewing, Just to let you know, you don't need to manually check phpcs or run the tests; the bot will do that for you and the box below the patch won't be green if there are coding standards issues or test failures - you also don't need to attach screenshots of the tests passing.

@gabriel.abdalla thank you for looking into this issue.

Posting screenshots of your codebase or command-line interface does not advance the issue, since the automated testing infrastructure tells us whether the change set still applies correctly.

So, I've removed the issue credit for that screenshot. In the future, you can get credit for issues by reading the issue to understand its purpose, and posting your review or testing of that purpose. Thank you!

RTBC'ing a comment changing issue like this one it is quite difficult to meet the standard that would be credited - the review doesn't deal with the most important thing to review which is that no new comments have been introduced using to drupal_render(). The review in #35 is a good example of a comment that will be credited.

Committed and pushed 84fc7490b1 to 9.3.x and f429dac245 to 9.2.x. Thanks!

As a patch that only changes comments backported to 9.2.x

We need a follow-up to do the same for references to render() since that function is now deprecated.

@alexpott Don't we usually only remove docs references once the deprecated function is actually removed?

@longwave not really. I think documentation should reflect the currently correct way of doing things. Telling people to use deprecated code is a recipe for them having to do future work.

@alexpott Maybe you can weigh in on #3224178: Remove theme function documentation from theme.api.php then ?