Problem/Motivation

Followup from #2704871: Replace usages of deprecated method drupal_render(). That issue replaces the core usages of the deprecated drupal_render() function, but does not yet update core string and documentation instances.

Proposed resolution

Update doc and string references to the service, class, etc. as appropriate. See child issues:

  1. #2938969: Replace drupal_render() in docblock and comments outside of @param, @return, @link, @see and outside of @code - @endcode
  2. #2938970: Replace drupal_render() in @param, @return, @see, @link, etc.
  3. #2938972: Replace drupal_render() in sample code
  4. #2938973: Replace drupal_render() within the Render component

Remaining tasks

TBD

CommentFileSizeAuthor
#41 interdiff-2834889-38-41.txt22.82 KBzeip
#41 2834889-41.patch59.92 KBzeip
#38 2834889-38.patch59.39 KBjofitz
#38 interdiff-32-38.txt4.29 KBjofitz
#32 2834889-32.patch58.66 KBjofitz
#32 interdiff-31-32.txt2.13 KBjofitz
#31 interdiff-26-31.txt33.45 KBzeip
#31 2834889-31.patch58.67 KBzeip
#29 2834889-29.patch48.87 KBzeip
#27 2834889-27.patch47.71 KBboaloysius
#26 interdiff-2834889-23-26.txt2.35 KBritzz
#26 2834889-26.patch47.42 KBritzz
#26 interdiff-2834889-23-25.txt2.35 KBritzz
#26 2834889-25.patch47.42 KBritzz
#23 interdiff-2834889-17-23.txt5.31 KBritzz
#23 2834889-23.patch47.41 KBritzz
#17 interdiff-2834889-15-17.txt5.93 KBwturrell
#17 2834889-17.patch47.53 KBwturrell
#15 interdiff-13-15.txt7.03 KBtameeshb
#15 2834889-15.patch45.77 KBtameeshb
#13 2834889-13.patch44.07 KBtameeshb
#13 interdiff-11-13.txt3.23 KBtameeshb
#11 2834889-11.patch44.07 KBtameeshb
#11 interdiff-9-11.txt0 bytestameeshb
#9 2834889-9.patch43.21 KBtameeshb
#4 core-drupal_render_document-2834889-4-D8.patch699 bytesdarrenwh
#3 replace drupal_render in document .patch666 bytesYasiru Nilan

Comments

xjm created an issue. See original summary.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Novice
Yasiru Nilan’s picture

Yasiru Nilan commented
StatusFileSize
new replace drupal_render in document .patch666 bytes

I'm new to drupal development, so I did a change as required in core/core.api.php file documentation.Please tell me is this correct before I proceed further

darrenwh’s picture

darrenwh commented
Status: Active » Needs review
StatusFileSize
new core-drupal_render_document-2834889-4-D8.patch699 bytes

Patch #1 went over 80 char limit, rerolled.

oo0shiny’s picture

oo0shiny commented
Status: Needs review » Reviewed & tested by the community

Tested patch locally and was able to see the correct changes. Setting this to RTBC.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Thanks for the patch!

This patch needs to replace all the documentation references to drupal_render(), not just one. There are lots more:

[ibnsina:drupal | Wed 20:11:28] $ grep -r "drupal_render" * | grep "\*"
core/includes/common.inc: * return drupal_render($table);
core/includes/common.inc: * links if drupal_render() is called on it, but calling drupal_render() on the
core/includes/common.inc: * @see drupal_render()
core/includes/common.inc: * The first time render() or drupal_render() is called on an element tree,
core/includes/common.inc: * render() or drupal_render() will not traverse the child tree of this element
core/includes/common.inc: * The first time render() or drupal_render() is called on an element tree,
core/includes/common.inc: * render() or drupal_render() will not traverse the child tree of this element
core/lib/Drupal/Core/Entity/Controller/EntityListController.php:   *   A render array as expected by drupal_render().
core/lib/Drupal/Core/Entity/Controller/EntityViewController.php:   *   A render array as expected by drupal_render().
core/lib/Drupal/Core/Entity/entity.api.php: * $rendered = drupal_render($build);
core/lib/Drupal/Core/Entity/entity.api.php: *   renderable array as expected by drupal_render().
core/lib/Drupal/Core/Entity/entity.api.php: *   renderable array as expected by drupal_render().
core/lib/Drupal/Core/Entity/entity.api.php: * drupal_render() for details.
core/lib/Drupal/Core/Entity/entity.api.php: * drupal_render() for details.
core/lib/Drupal/Core/Entity/entity.api.php: * Alter entity renderable values before cache checking in drupal_render().
core/lib/Drupal/Core/Entity/entity.api.php: * @see drupal_render()
core/lib/Drupal/Core/Entity/entity.api.php: * Alter entity renderable values before cache checking in drupal_render().
core/lib/Drupal/Core/Entity/entity.api.php: * @see drupal_render()
core/lib/Drupal/Core/Entity/EntityListBuilderInterface.php:   *   A render array as expected by drupal_render().
core/lib/Drupal/Core/Entity/EntityViewBuilder.php:   * @see drupal_render()
core/lib/Drupal/Core/Entity/EntityViewBuilder.php:   * drupal_render(), the processing cost of assembling an entity's renderable
core/lib/Drupal/Core/Entity/EntityViewBuilder.php:   * @see drupal_render()
core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php: * When controllers call drupal_render() (RendererInterface::render()) outside
core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php: * drupal_render(), and bubbleable metadata was collected. This bubbleable
core/lib/Drupal/Core/Form/FormBuilderInterface.php:   * to invoke validation logic for each element) and drupal_render() (for
core/lib/Drupal/Core/Form/FormBuilderInterface.php:   * self::doValidateForm() and drupal_render(), appropriate for those
core/lib/Drupal/Core/Menu/menu.api.php: * $menu_html = drupal_render($menu);
core/lib/Drupal/Core/Render/Element.php:   * not start with a '#'. See drupal_render() for details.
core/lib/Drupal/Core/Render/MainContent/AjaxRenderer.php:   * Wraps drupal_render_root().
core/lib/Drupal/Core/Render/theme.api.php: * to drupal_render(), traversing the depth of the render array hierarchy. At
core/lib/Drupal/Core/Render/theme.api.php: * documentation of drupal_render() and the
core/lib/Drupal/Core/Render/theme.api.php: * services will eventually be rendered by a call to drupal_render(), which will
core/lib/Drupal/Core/Render/theme.api.php: * would be to create a render array, render it by calling drupal_render(), and
core/lib/Drupal/Core/Render/theme.api.php: * documentation of drupal_render() for more details of the rendering process.
core/modules/aggregator/src/Controller/AggregatorController.php:   *   A form array as expected by drupal_render().
core/modules/aggregator/src/Controller/AggregatorController.php:   *   A render array as expected by drupal_render().
core/modules/block/block.api.php: * block.html.twig. See drupal_render() documentation or the
core/modules/block/src/Controller/BlockListController.php:   *   A render array as expected by drupal_render().
core/modules/block_content/src/Controller/BlockContentController.php:   *   A form array as expected by drupal_render().
core/modules/book/src/BookManagerInterface.php:   *   A structured array to be rendered by drupal_render().
core/modules/comment/comment.module: *   An array as expected by drupal_render().
core/modules/comment/comment.module: *   An array in the format expected by drupal_render().
core/modules/comment/comment.module: * @see drupal_render()
core/modules/comment/comment.module: *   An array as expected by drupal_render().
core/modules/config_translation/src/Controller/ConfigTranslationListController.php:   *   A render array as expected by drupal_render().
core/modules/contact/src/Controller/ContactController.php:   *   The form as render array as expected by drupal_render().
core/modules/contact/src/Controller/ContactController.php:   *   The personal contact form as render array as expected by drupal_render().
core/modules/dblog/src/Controller/DbLogController.php:   *   A render array as expected by drupal_render().
core/modules/dblog/src/Controller/DbLogController.php:   *   format expected by drupal_render();
core/modules/dblog/src/Controller/DbLogController.php:   *   A build array in the format expected by drupal_render().
core/modules/field_ui/src/Controller/FieldConfigListController.php:   *   A render array as expected by drupal_render().
core/modules/field_ui/src/Element/FieldUiTable.php:   * @see drupal_render()
core/modules/field_ui/src/Form/EntityDisplayFormBase.php:   * @see drupal_render()
core/modules/help/src/Controller/HelpController.php:   *   A render array as expected by drupal_render().
core/modules/image/src/Tests/ImageDimensionsTest.php:   * drupal_render() alters the passed $variables array by adding a new key
core/modules/image/src/Tests/ImageDimensionsTest.php:   * wrap drupal_render() in a helper protected method and pass each time a
core/modules/node/node.module: *   An array as expected by drupal_render().
core/modules/node/node.module: * Constructs a drupal_render() style array from an array of loaded nodes.
core/modules/node/node.module: *   An array in the format expected by drupal_render().
core/modules/node/src/Controller/NodeController.php:   *   An array suitable for drupal_render().
core/modules/node/src/Controller/NodeController.php:   *   An array as expected by drupal_render().
core/modules/path/src/Controller/PathController.php:   *   A render array as expected by drupal_render().
core/modules/simpletest/src/WebTestBase.php:   * renderable array for an entity is constructed in drupal_render(). Some
core/modules/simpletest/src/WebTestBase.php:   * drupal_render process in order to verify the presence of specific values.
core/modules/simpletest/src/WebTestBase.php:   * @see drupal_render()
core/modules/system/src/SystemManager.php:   *   A render array suitable for drupal_render.
core/modules/system/src/Tests/Common/RenderWebTest.php: * Performs integration tests on drupal_render().
core/modules/system/tests/modules/common_test/common_test.module: * Provides a theme function for drupal_render().
core/modules/system/tests/modules/test_page_test/src/Controller/Test.php:   *   A render array as expected by drupal_render()
core/modules/system/tests/modules/test_page_test/src/Controller/Test.php:   *   A render array as expected by drupal_render().
core/modules/system/tests/modules/test_page_test/src/Controller/Test.php:   *   A render array as expected by drupal_render()
core/modules/system/tests/modules/test_page_test/src/Controller/Test.php:   *   A render array as expected by drupal_render()
core/modules/system/tests/modules/test_page_test/src/Controller/Test.php:   *   A render array as expected by drupal_render()
core/modules/system/tests/modules/theme_test/theme_test.module: * Theme function for testing rendering of child elements via drupal_render().
core/modules/system/tests/modules/theme_test/theme_test.module: * property. When this property is found, drupal_render() avoids calling
core/modules/taxonomy/taxonomy.module: *   A $page element suitable for use by drupal_render().
core/modules/taxonomy/taxonomy.module: * Constructs a drupal_render() style array from an array of loaded terms.
core/modules/taxonomy/taxonomy.module: *   An array in the format expected by drupal_render().
core/modules/toolbar/src/Element/Toolbar.php:   * Builds the Toolbar as a structured array ready for drupal_render().
core/modules/toolbar/toolbar.module: * @see drupal_render()
core/modules/user/user.module: *   An array as expected by drupal_render().
core/modules/user/user.module: * Constructs a drupal_render() style array from an array of loaded users.
core/modules/user/user.module: *   An array in the format expected by drupal_render().
core/modules/views/src/Plugin/views/field/FieldHandlerInterface.php:   * Passes values to drupal_render() using $this->themeFunctions() as #theme.
core/modules/views/views.module: *   should be suitable for passing in to drupal_render() and will normally
core/tests/Drupal/KernelTests/Core/Render/Element/RenderElementTypesTest.php: * Tests the markup of core render element types passed to drupal_render().
core/tests/Drupal/KernelTests/Core/Render/RenderTest.php: * Performs functional tests on drupal_render().
core/themes/seven/seven.theme: * because they will be processed by Twig and drupal_render will

So, let's go ahead and correct the others too. :)

tameeshb’s picture

tameeshb commented
Assigned: Unassigned » tameeshb
tameeshb’s picture

tameeshb commented
Status: Needs work » Needs review
StatusFileSize
new 2834889-9.patch43.21 KB
manuel garcia’s picture

manuel garcia commented
Status: Needs review » Needs work

Thank you @tameeshb for working on this.

+++ b/core/includes/common.inc
@@ -762,7 +762,7 @@ function drupal_pre_render_link($element) {
+ * links if \Drupal::service('renderer')->render() is called on it, but calling \Drupal::service('renderer')->render() on the

Documentation lines must be under 80 characters, this happens throughout the patch.
https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...

tameeshb’s picture

tameeshb commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-9-11.txt0 bytes
new 2834889-11.patch44.07 KB

Revised patch uploaded, please review. :)

manuel garcia’s picture

manuel garcia commented
Status: Needs review » Needs work

Thanks @tameeshb! Some more coding standards nitpicks:

+++ b/core/includes/common.inc
@@ -941,10 +942,12 @@ function render(&$element) {
+ * it is marked with a #printed flag and the rendered children of ¶
+ * the element are cached. Subsequent calls to render() or ¶
+ * \Drupal::service('renderer')->render() will not traverse the ¶

Trailing spaces on these lines need to be removed. There are a few others throughout the patch.

tameeshb’s picture

tameeshb commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-11-13.txt3.23 KB
new 2834889-13.patch44.07 KB

Removed all trailing spaces in the comments.

manuel garcia’s picture

manuel garcia commented
Status: Needs review » Needs work

Thanks! nearly there:

  1. +++ b/core/includes/common.inc
@@ -970,10 +973,12 @@ function hide(&$element) {
+ * the child tree of this element
  * again: they will just use the cached children. So if you want to show an
  * element, be sure to call show() on the element before its parent tree is
  * rendered for the first time, as it will have no effect on subsequent

    We make this line longer with the rest of the content that follows. 

  2. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -1485,7 +1486,8 @@ function hook_ENTITY_TYPE_build_defaults_alter(array &$build, \Drupal\Core\Entit
+ *\Drupal::service('renderer')->render().

    There needs to be a space after the asterisk. 

  3. +++ b/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php
@@ -17,7 +17,8 @@
+ * (RendererInterface::render()) outside
  * of a render context, we call that "early rendering". Controllers should
  * return only render arrays, but we cannot prevent controllers from doing early
  * rendering. The problem with early rendering is that the bubbleable metadata

    We can make this line longer with the rest of the text that follows. 

  4. +++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
@@ -244,8 +244,9 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state);
+   * Each of these three pipelines provides ample
    * opportunity for modules to customize what happens. For example, during this
    * function's life cycle, the following functions get called for each element:

    We can make this line longer with the rest of the text that follows. 

  5. +++ b/core/modules/block/block.api.php
@@ -72,7 +72,8 @@
+ * documentation or the
  * @link themeable Default theme implementations topic @endlink for details.

    We can make this line longer with the rest of the text that follows. 

  6. +++ b/core/modules/image/src/Tests/ImageDimensionsTest.php
@@ -277,9 +277,11 @@ function testImageDimensions() {
+   * by adding a new key
    * '#printed' => TRUE. This prevents next call to re-render the element. We
-   * wrap drupal_render() in a helper protected method and pass each time a
+   * wrap \Drupal::service('renderer')->render() in a helper protected method
+   * and pass each time a
    * fresh array so that $variables won't get altered and the element is
    * re-rendered each time.

    We can make these lines longer with the rest of the text that follows. 

  7. +++ b/core/modules/simpletest/src/WebTestBase.php
@@ -214,7 +214,8 @@ function __construct($test_id = NULL) {
+   * \Drupal::service('renderer')->render(). Some
    * tests require the complete renderable array for an entity outside of the
    * drupal_render process in order to verify the presence of specific values.

    We can make this line longer with the rest of the text that follows. 

  8. +++ b/core/themes/seven/seven.theme
@@ -26,7 +26,8 @@ function seven_preprocess_html(&$variables) {
+ * \Drupal::service('renderer')->render will
  * be invoked.

    Can we make this one line (there is space and it would read better).

tameeshb’s picture

tameeshb commented
StatusFileSize
new 2834889-15.patch45.77 KB
new interdiff-13-15.txt7.03 KB

8/8 changes done. Please re-review!

tameeshb’s picture

tameeshb commented
Status: Needs work » Needs review
wturrell’s picture

wturrell commented
StatusFileSize
new 2834889-17.patch47.53 KB
new interdiff-2834889-15-17.txt5.93 KB

Fixes and partial review:

This is a tedious job (as I now appreciate!) so well done on completing it…

- Patch applies cleanly (8.3.x)
- I've read every change in the 40+ files and viewed them in PhpStorm
- I'm not seeing any coding style / inspection warnings etc.
- I've fixed a couple of occurrences that were missed
- Fixed some line length/indentation issues

NB: There's a small number of inline code comments in /core referring to drupal_render(), but which aren't next to an actual usage of it - I've left those, to stay in scope.

Documentation rather than code, so:
- can't see this would create any regressions
- no translatable string changes

Someone will now need to check my changes then hopefully we can get this into 8.3.

Also we need to wait for the automated tests to run once the branch has been fixed.

wturrell’s picture

wturrell commented
Assigned: tameeshb » Unassigned
manuel garcia’s picture

manuel garcia commented

Brilliant, thank you @tameeshb & @wturrell

I've reviewed the interdiffs on #15 & #17, and they look good to me.

tameeshb’s picture

tameeshb commented

:)

manuel garcia’s picture

manuel garcia commented
Status: Needs review » Reviewed & tested by the community

Patch came back green =)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Here's the start of a review... a couple of general points... we need to point to \Drupal\Core\Render\RendererInterface::render() in the @see's so api.d.o can link up the docs. Also when referring to the array structure expected by ::render() we should also point to the interface. When we are talking about an implementation detail then pointing to \Drupal::service('renderer')->render() or \Drupal\Core\Render\Renderer::render() is appropriate. Another useful thing to link to is https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21... - you can do that in docs with @link theme_render Render API @endlink.

Unfortunately this task is not a simple string replace.

  1. +++ b/core/includes/common.inc
@@ -887,7 +888,7 @@ function drupal_render(&$elements, $is_recursive_call = FALSE) {
+ * @see \Drupal::service('renderer')->render()

    This should be an @see to \Drupal\Core\Render\RendererInterface::render 

  2. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityListController.php
@@ -16,7 +16,7 @@ class EntityListController extends ControllerBase {
+   *   A render array as expected by \Drupal::service('renderer')->render().

+++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
@@ -88,7 +88,7 @@ public function buildTitle(array $page) {
+   *   A render array as expected by \Drupal::service('renderer')->render().

+++ b/core/lib/Drupal/Core/Entity/EntityListBuilderInterface.php
@@ -45,7 +45,7 @@ public function getOperations(EntityInterface $entity);
+   *   A render array as expected by \Drupal::service('renderer')->render().

    as expected by \Drupal\Core\Render\RendererInterface::render(). 

  3. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -213,7 +213,7 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode) {
+   * @see \Drupal::service('renderer')->render()

    See @see comment before. 

  4. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -237,7 +237,7 @@ public function build(array $build) {
+   * @see \Drupal::service('renderer')->render()

    See @see comment... 

  5. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -1459,7 +1459,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().

@@ -1485,7 +1486,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().

    This should be on one line... going to be tricky. 

  6. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -1474,7 +1475,7 @@ function hook_entity_view_mode_alter(&$view_mode, Drupal\Core\Entity\EntityInter
+ * @see \Drupal::service('renderer')->render()

    Again...

ritzz’s picture

ritzz commented
Assigned: Unassigned » ritzz
Status: Needs work » Needs review
StatusFileSize
new 2834889-23.patch47.41 KB
new interdiff-2834889-17-23.txt5.31 KB 
@@ -1485,7 +1486,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().

Could not fit this in one line. Any way to do it?

manuel garcia’s picture

manuel garcia commented
Status: Needs review » Needs work 
  1. +++ b/core/includes/common.inc
@@ -888,7 +888,7 @@
- * @see \Drupal::service('renderer')->render()
+ * @see to \Drupal\Core\Render\RendererInterface::render().

    no need for 'to' 

  2. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -213,7 +213,7 @@
-   * @see \Drupal::service('renderer')->render()
+   * @see to \Drupal\Core\Render\RendererInterface::render()

    no need for 'to' 

  3. +++ b/core/lib/Drupal/Core/Entity/EntityViewBuilder.php
@@ -237,7 +237,7 @@
-   * @see \Drupal::service('renderer')->render()
+   * @see to \Drupal\Core\Render\RendererInterface::render()

    no need for 'to' 

  4. +++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -1475,7 +1475,7 @@
- * @see \Drupal::service('renderer')->render()
+ * @see to \Drupal\Core\Render\RendererInterface::render().

    no need for 'to' 

  5. +++ b/core/modules/comment/comment.module
@@ -253,7 +253,7 @@
- *   An array as expected by \Drupal::service('renderer')->render().
+ *   An array as expected by drupal_render().

    Should be 'An array as expected by \Drupal\Core\Render\RendererInterface::render()' 

  6. +++ b/core/modules/comment/comment.module
@@ -262,7 +262,7 @@
- *   An array as expected by drupal_render().
+ *   An array as expected by \Drupal::service('renderer')->render().

    Should be 'An array as expected by \Drupal\Core\Render\RendererInterface::render()'

tameeshb’s picture

tameeshb commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-23-25.txt2.66 KB
new 2834889-25.patch47.41 KB

Made changes from #24

ritzz’s picture

ritzz commented
StatusFileSize
new 2834889-25.patch47.42 KB
new interdiff-2834889-23-25.txt2.35 KB
new 2834889-26.patch47.42 KB
new interdiff-2834889-23-26.txt2.35 KB
boaloysius’s picture

boaloysius commented
StatusFileSize
new 2834889-27.patch47.71 KB

patch reroll

boaloysius’s picture

boaloysius commented
zeip’s picture

zeip commented
StatusFileSize
new 2834889-29.patch48.87 KB

Re-rolling the patch again for 8.3.x. Also replaced mentions in core/tests/Drupal/Tests/EntityViewTrait.php, core/core.api.php and core/modules/path/src/Controller/PathController.php.

zeip’s picture

zeip commented
Assigned: ritzz » Unassigned
zeip’s picture

zeip commented
StatusFileSize
new 2834889-31.patch58.67 KB
new interdiff-26-31.txt33.45 KB

Attached is a patch with a few more string replaces. Now all drupal_render() instances are replaced by either this or #2704871: Replace usages of deprecated method drupal_render(). Should be finally read for the review.

jofitz’s picture

jofitz
he/him
commented
StatusFileSize
new interdiff-31-32.txt2.13 KB
new 2834889-32.patch58.66 KB

Minor coding standards corrections to @see.

vegantriathlete’s picture

vegantriathlete
he/him/his
Primary language English
Location Lakewood, CO
commented
Issue tags: +dcco2017

see if you can get it to RTBC

crossfish’s picture

crossfish commented

Ran on SimplyTest.me and applied patch #32 to local dev environment and all modifications to comments did not cause any problems. Reviewed comments and code changes and proper Drupal standards were applied.

crossfish’s picture

crossfish commented
Status: Needs review » Reviewed & tested by the community
manuel garcia’s picture

manuel garcia commented
Status: Reviewed & tested by the community » Needs work

Thank you crossfish for testing!

Pushing this back - we need to first adjust these comment blocks:

  1. +++ b/core/includes/common.inc
@@ -970,10 +973,12 @@ function hide(&$element) {
- * The first time render() or drupal_render() is called on an element tree,
+ * The first time render() or \Drupal::service('renderer')->render()
+ * is called on an element tree,
  * as each element in the tree is rendered, it is marked with a #printed flag
  * and the rendered children of the element are cached. Subsequent calls to
- * render() or drupal_render() will not traverse the child tree of this element
+ * render() or \Drupal::service('renderer')->render() will not traverse
+ * the child tree of this element
  * again: they will just use the cached children. So if you want to show an

    Can we fill up to the end of the line please 

  2. +++ b/core/lib/Drupal/Core/Render/theme.api.php
@@ -232,13 +233,16 @@
- * would be to create a render array, render it by calling drupal_render(), and
+ * would be to create a render array, render it by calling
+ * \Drupal::service('renderer')->render(), and
  * return that result, rather than writing the markup directly. See the

    Same

vegantriathlete’s picture

vegantriathlete
he/him/his
Primary language English
Location Lakewood, CO
commented
Issue tags: -dcco2017
jofitz’s picture

jofitz
he/him
commented
Status: Needs work » Needs review
Issue tags: -Novice
StatusFileSize
new interdiff-32-38.txt4.29 KB
new 2834889-38.patch59.39 KB

Corrected line-breaks in comments as highlighted in #36.

manuel garcia’s picture

manuel garcia commented

Thanks! Tempted to to RTBC again, but going back to @alexpott's review on #22, I had a look at the patch trying to identify where we should point to the interface instead. Here are some places that could be useful to do so at a first glance:

+++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -1264,7 +1264,7 @@ function hook_ENTITY_TYPE_revision_delete(Drupal\Core\Entity\EntityInterface $en
+ *   renderable array as expected by \Drupal::service('renderer')->render().

@@ -1296,7 +1296,7 @@ function hook_entity_view(array &$build, \Drupal\Core\Entity\EntityInterface $en
+ *   renderable array as expected by \Drupal::service('renderer')->render().

+++ b/core/modules/aggregator/src/Controller/AggregatorController.php
@@ -44,7 +44,7 @@ public static function create(ContainerInterface $container) {
+   *   A form array as expected by \Drupal::service('renderer')->render().

@@ -104,7 +104,7 @@ public function feedRefresh(FeedInterface $aggregator_feed) {
+   *   A render array as expected by \Drupal::service('renderer')->render().

+++ b/core/modules/block/src/Controller/BlockListController.php
@@ -48,7 +48,7 @@ public static function create(ContainerInterface $container) {
+   *   A render array as expected by \Drupal::service('renderer')->render().

+++ b/core/modules/block_content/src/Controller/BlockContentController.php
@@ -98,7 +98,7 @@ public function add(Request $request) {
+   *   A form array as expected by \Drupal::service('renderer')->render().

+++ b/core/modules/book/src/BookManagerInterface.php
@@ -247,7 +247,8 @@ public function deleteFromBook($nid);
+   *   \Drupal::service('renderer')->render().

+++ b/core/modules/comment/comment.module
@@ -285,12 +285,12 @@ function comment_view(CommentInterface $comment, $view_mode = 'full', $langcode
+ *   An array in the format expected by \Drupal::service('renderer')->render().

+++ b/core/modules/config_translation/src/Controller/ConfigTranslationListController.php
@@ -45,7 +45,7 @@ public static function create(ContainerInterface $container) {
+   *   A render array as expected by \Drupal::service('renderer')->render().

+++ b/core/modules/contact/src/Controller/ContactController.php
@@ -47,7 +47,8 @@ public static function create(ContainerInterface $container) {
+   *   \Drupal::service('renderer')->render().

@@ -94,7 +95,8 @@ public function contactSitePage(ContactFormInterface $contact_form = NULL) {
+   *   \Drupal::service('renderer')->render().

+++ b/core/modules/dblog/src/Controller/DbLogController.php
@@ -113,7 +113,7 @@ public static function getLogLevelClassMap() {
+   *   A render array as expected by \Drupal::service('renderer')->render().

@@ -233,7 +233,7 @@ public function overview() {
+   *   format expected by \Drupal::service('renderer')->render();

@@ -373,7 +373,8 @@ public function formatMessage($row) {
+   *   \Drupal::service('renderer')->render().

+++ b/core/modules/field_ui/src/Controller/FieldConfigListController.php
@@ -21,7 +21,7 @@ class FieldConfigListController extends EntityListController {
+   *   A render array as expected by \Drupal::service('renderer')->render().

+++ b/core/modules/help/src/Controller/HelpController.php
@@ -107,7 +107,7 @@ public function helpMain() {
+   *   A render array as expected by \Drupal::service('renderer')->render().

+++ b/core/modules/node/node.module
@@ -794,14 +794,15 @@ function node_get_recent($number = 10) {
+ *   An array as expected by \Drupal::service('renderer')->render().
...
+ * Constructs a \Drupal::service('renderer')->render() style array from an

@@ -812,7 +813,7 @@ function node_view(NodeInterface $node, $view_mode = 'full', $langcode = NULL) {
+ *   An array in the format expected by \Drupal::service('renderer')->render().

+++ b/core/modules/node/src/Controller/NodeController.php
@@ -122,7 +122,7 @@ public function add(NodeTypeInterface $node_type) {
+   *   An array suitable for \Drupal::service('renderer')->render().

@@ -154,7 +154,7 @@ public function revisionPageTitle($node_revision) {
+   *   An array as expected by \Drupal::service('renderer')->render().

+++ b/core/modules/path/src/Controller/PathController.php
@@ -59,7 +59,7 @@ public static function create(ContainerInterface $container) {
+   *   A render array as expected by \Drupal::service('renderer')->render().

+++ b/core/modules/system/src/SystemManager.php
@@ -152,7 +152,7 @@ public function getMaxSeverity(&$requirements) {
+   *   A render array suitable for \Drupal::service('renderer')->render.

+++ b/core/modules/system/tests/modules/test_page_test/src/Controller/Test.php
@@ -12,7 +12,7 @@ class Test {
+   *   A render array as expected by \Drupal::service('renderer')->render()

@@ -26,7 +26,7 @@ public function renderTitle() {
+   *   A render array as expected by \Drupal::service('renderer')->render().

@@ -63,7 +63,7 @@ public function controllerWithCache() {
+   *   A render array as expected by \Drupal::service('renderer')->render()

@@ -92,7 +92,7 @@ public function error() {
+   *   A render array as expected by \Drupal::service('renderer')->render()

@@ -102,7 +102,7 @@ public function renderEncodedMarkup() {
+   *   A render array as expected by \Drupal::service('renderer')->render()

+++ b/core/modules/taxonomy/taxonomy.module
@@ -195,14 +195,15 @@ function taxonomy_check_vocabulary_hierarchy(VocabularyInterface $vocabulary, $c
+ *   A $page element suitable for use by \Drupal::service('renderer')->render().
...
+ * Constructs a \Drupal::service('renderer')->render() style array from an

@@ -213,7 +214,7 @@ function taxonomy_term_view(Term $term, $view_mode = 'full', $langcode = NULL) {
+ *   An array in the format expected by \Drupal::service('renderer')->render().

+++ b/core/modules/toolbar/src/Element/Toolbar.php
@@ -50,7 +50,8 @@ public function getInfo() {
+   * \Drupal::service('renderer')->render().

+++ b/core/modules/toolbar/toolbar.module
@@ -217,7 +217,7 @@ function toolbar_toolbar() {
+ * @see \Drupal::service('renderer')->render()

+++ b/core/modules/user/user.module
@@ -893,14 +893,15 @@ function user_delete_multiple(array $uids) {
+ *   An array as expected by \Drupal::service('renderer')->render().

@@ -911,7 +912,7 @@ function user_view($account, $view_mode = 'full', $langcode = NULL) {
+ *   An array in the format expected by \Drupal::service('renderer')->render().

+++ b/core/modules/views/views.module
@@ -322,9 +322,9 @@ function views_theme_suggestions_container_alter(array &$suggestions, array $var
+ *   should be suitable for passing in to \Drupal::service('renderer')->render()

+++ b/core/tests/Drupal/Tests/EntityViewTrait.php
@@ -32,7 +32,7 @@
+   * @see \Drupal::service('renderer')->render()

Should we be pointing to \Drupal\Core\Render\RendererInterface::render() here instead?

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

zeip’s picture

zeip commented
StatusFileSize
new 2834889-41.patch59.92 KB
new interdiff-2834889-38-41.txt22.82 KB

Thanks for the review! Attached is a new patch changing those and a few other instances of the service reference to the interface. I changed most of the return value descriptions (the ones not containing any additional information) to all conform to the same one-row format.

Would this be ok or are we still missing something?

manuel garcia’s picture

manuel garcia commented
Status: Needs review » Needs work

Thanks @ZeiP for that!

Had a quick look at the interdiff:

+++ b/core/core.api.php
@@ -2441,8 +2441,8 @@
+ * to a render array, use drupal_render() to render it, and add a command to

+++ b/core/modules/book/src/BookManager.php
@@ -504,8 +504,7 @@
+      // Make sure drupal_render() does not re-order the links.

+++ b/core/modules/comment/comment.module
@@ -565,7 +565,7 @@
+ *   An array as expected by drupal_render().

We should not be referring to drupal_render() here.

zeip’s picture

zeip commented
Status: Needs work » Needs review

Thanks! We aren't, the patch itself doesn't contain those rows but for some reason the interdiff has a few incorrect lines. I don't know why, since most of the interdiff lines are correct and I generated it with the patchutils interdiff as usual.

Setting back to Needs review.

manuel garcia’s picture

manuel garcia commented
Status: Needs review » Reviewed & tested by the community

Ow OK, yup, you're right those changes are not in the patch.

I had another read at all the things, and I believe we are ready for committer review again... so RTBCing =)

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

So a couple things:

  1. In a number of places we're cleverly rewording comments to make stuff fit within 80 chars without wrapping, but changing the wording of the comments is out of scope. The patch here should only replace references drupal_render() itself and not make other changes other than rewrapping; rewriting comments makes the word diff no longer as useful and requires a different review scope because then you have to read the whole comment and make sure it's still true and grammatically correct.
  2. ...Except for one-line summaries, where it's a hard requirement that it be a single line of 80 characters or fewer. E.g. this does not follow our standard:
    +++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -1464,7 +1466,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\Core\Render\RendererInterface::render().
  3. As mentioned in #22, there are a couple different possibilities for what the documentation should refer to. Looking through the patch, I think a few of them are wrong; generally, in code where the renderer service would probably be injected,
    we shouldn't refer to the \Drupal::service() convenience wrapper. Or, in other words, we should probably only refer to \Drupal::service('renderer') if we're in a procedural context. I especially don't think we should be referring to it within the Render component itself. Those should refer to the interface method instead, most likely.
  4. Furthermore, some of the hunks in this patch also don't follow the guidelines laid out above and aren't even consistent with comparable hunks. These are wrong, for example:
    +++ b/core/lib/Drupal/Core/Entity/entity.api.php
@@ -1340,7 +1342,7 @@ function hook_ENTITY_TYPE_view(array &$build, \Drupal\Core\Entity\EntityInterfac
  * See the @link themeable Default theme implementations topic @endlink and
- * drupal_render() for details.

@@ -1378,7 +1380,7 @@ function hook_entity_view_alter(array &$build, Drupal\Core\Entity\EntityInterfac
  * See the @link themeable Default theme implementations topic @endlink and
- * drupal_render() for details.
+ * \Drupal::service('renderer')->render() for details.

    .

  5. The patch is borderline-unwieldy for a cleanup at 60K.

Based on the above, I think we can split this up into a small number of sub-issues:

  1. Rewrite one-line summaries that refer to drupal_render(), since unlike most things these aren't going to work as straight string-replaces. They probably shouldn't refer to it anyway; it's an implementation detail. Usually, "during rendering" or such is probably an acceptable substitution in one-line summaries.
  2. Replace references to drupal_render() with RenderInterface::render() in @param, @return, @see, @link, etc. references.
  3. Replace references to drupal_render() with \Drupal::service()... in sample code (*.api.php, @code/@endcode, etc.)
  4. Replace references to drupal_render() with RenderInterface::render() within the Render component.
  5. Sort out whatever's left.

Thanks!

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jofitz’s picture

jofitz
he/him
commented
Issue summary: View changes
Status: Needs work » Postponed

Created child issues:

  1. #2938969: Replace drupal_render() in docblock and comments outside of @param, @return, @link, @see and outside of @code - @endcode
  2. #2938970: Replace drupal_render() in @param, @return, @see, @link, etc.
  3. #2938972: Replace drupal_render() in sample code
  4. #2938973: Replace drupal_render() within the Render component

Postponing this ticket until all 4 have been completed at which point we can:

5. Sort out whatever's left.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Adding credit to make copying to the sub issues easier. Crediting @xjm, @Manuel Garcia and myself for reviews that directly influenced the patch.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Issue tags: +Needs reroll
avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Version: 8.6.x-dev » 8.9.x-dev

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone commented
Status: Postponed » Closed (outdated)
Issue tags: -Needs reroll

Searched core and there is only one instance of 'drupal_render'. 

$ git grep drupal_render
core/modules/system/tests/modules/common_test/common_test.module:function common_test_drupal_render_printing_pre_render($elements) {

I then found that the use of that function was removed in #2378883: Convert existing drupal_render() KernelTestBase tests to PHPUnit tests. It appears to be dead code. I made an issue for that #3305648: Remove common_test_drupal_render_printing_pre_render

So, this can be closed as outdated.

Thanks everyone!