Parent issue #2393329: Replace all drupal_render calls with the service, and inject it, if possible.

Problem/Motivation

drupal_render() was marked as deprecated, though its still called in quite some places.

Proposed resolution

  1. Avoid rendering manually by letting the template who is printing the variable render it.
  2. Inject the renderer service into service, which uses drupal_render()
  3. Use \Drupal::service('renderer')->render() for old prodecural code.

Remaining tasks

  1. Find all the actionable fixes: Search for drupal_render(
  2. Create a Patch
  3. Review

User interface changes

None

API changes

None

Files: 
CommentFileSizeAuthor
#18 replace_all-2472975-18.patch3.63 KBdimaro
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,427 pass(es). View
#18 interdiff-2472975-14-18.txt1.5 KBdimaro
#14 interdiff-2472975-4-5.txt1.8 KBRenrhaf
#14 ckeditor_replace_drupal_render-2472975-5.patch2.78 KBRenrhaf
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,384 pass(es). View
#4 interdiff-ckeditor_replace_drupal_render-2472975-1-4.txt2.42 KBkeopx
#4 ckeditor_replace_drupal_render-2472975-4.patch2.74 KBkeopx
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,576 pass(es). View
#1 ckeditor_replace_drupal_render-2472975-1.patch698 byteskeopx
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,561 pass(es). View

Comments

keopx’s picture

Assigned: keopx » Unassigned
Status: Active » Needs review
FileSize
698 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,561 pass(es). View
mitrpaka’s picture

Status: Needs review » Needs work

Thanks for the patch. In case of plugins it is recommended to use dependency injection (rather than using out service directly through the global services container).

See #2471913: Replace drupal_render calls in core/modules/simpletest as an example (e.g. SimpletestTestForm.php).

keopx’s picture

Assigned: Unassigned » keopx

Ok, thanks for information and reference

keopx’s picture

Assigned: keopx » Unassigned
Status: Needs work » Needs review
FileSize
2.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,576 pass(es). View
2.42 KB
cilefen’s picture

Status: Needs review » Needs work
+++ b/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php
@@ -13,6 +13,7 @@
 use Drupal\Core\Render\Element;
+use Drupal\Core\Render\Renderer;
 use Drupal\editor\Plugin\EditorBase;

@@ -56,6 +57,13 @@ class CKEditor extends EditorBase implements ContainerFactoryPluginInterface {
   /**
+   * Rendering service.
+   *
+   * @var \Drupal\Core\Render\Renderer
+   */
+  protected $renderer;

@@ -70,12 +78,15 @@ class CKEditor extends EditorBase implements ContainerFactoryPluginInterface {
    * @param \Drupal\Core\Language\LanguageManagerInterface $language_manager
    *   The language manager.
+   * @param \Drupal\Core\Render\Renderer $renderer
+   *   The renderer service.
    */
-  public function __construct(array $configuration, $plugin_id, $plugin_definition, CKEditorPluginManager $ckeditor_plugin_manager, ModuleHandlerInterface $module_handler, LanguageManagerInterface $language_manager) {
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, CKEditorPluginManager $ckeditor_plugin_manager, ModuleHandlerInterface $module_handler, LanguageManagerInterface $language_manager, Renderer $renderer) {

Thanks for working on this. Two things: You must type hint to the interface, so it should be RendererInterface.

Second, there are some comments in ckeditor.module that were missed. They may need changing.

Renrhaf’s picture

FileSize
2.78 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,435 pass(es). View

Hello,

I'm novice here in contributing so I took this easy task for a start.
I hope you don't mind, @keopx, that I continued this little task ?

Here is the patch for the Interface usage instead of the real class.
@cilefen : I didn't understand your feedback about comments, can you tell me more ?

Regards,
Renrhaf

Renrhaf’s picture

Status: Needs work » Needs review
cilefen’s picture

Hi @Renrhaf. Thanks for contributing!

Good work on the type hinting. My comment means this:

$ grep -r 'drupal_render(' core/modules/ckeditor/
core/modules/ckeditor//ckeditor.module: * If this wouldn't happen in hook_rebuild(), then the first drupal_render()
core/modules/ckeditor//ckeditor.module: * check_markup(), which finally calls drupal_render() non-recursively, because
core/modules/ckeditor//ckeditor.module: * check_markup() calls outside of a drupal_render() call tree.

As far as I know, we are changing comments too. That is not a law though - you could start a discussion on the parent issue about it.

Since you are new I don't want to pile more requirements on. However, it is appreciated if you could upload an interdiff.txt with your next patches. (see https://www.drupal.org/documentation/git/interdiff).

cilefen’s picture

Status: Needs review » Needs work
+++ b/core/modules/ckeditor/src/Plugin/Editor/CKEditor.php
@@ -56,6 +57,13 @@ class CKEditor extends EditorBase implements ContainerFactoryPluginInterface {
+   * Rendering service.

I would make this "The renderer service." to be in line with the usual way we describe these and the constructor comment.

keopx’s picture

@Renrhaf not problem and you are welcome, but please use "Assigned" to know you are working on it and remove it when you leave ;)

Renrhaf’s picture

Assigned: Unassigned » Renrhaf

Right I will do it as soon as possible today !

Renrhaf’s picture

Assigned: Renrhaf » Unassigned
Status: Needs work » Needs review
FileSize
2.8 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: failed to drop checkout database. View
1.8 KB

Here is my updated patch and the interdiff ! Hope it's OK ;)

Status: Needs review » Needs work

The last submitted patch, 12: ckeditor_replace_drupal_render-2472975-5.patch, failed testing.

Renrhaf’s picture

FileSize
2.78 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,384 pass(es). View
1.8 KB

Fixing my error

Renrhaf’s picture

Status: Needs work » Needs review
dimaro’s picture

Assigned: Unassigned » dimaro
Status: Needs review » Needs work
dimaro’s picture

dimaro’s picture

Assigned: dimaro » Unassigned
Status: Needs work » Needs review
FileSize
1.5 KB
3.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,427 pass(es). View

Delete "use Drupal \ Core \ Cache \ CacheBackendInterface;" because it is not used.

Moreover, I think we should use the same syntaxis in the same method, see interdiff.

cilefen’s picture

Status: Needs review » Needs work

@dimaro The array syntax change is not wrong, but it is an unrelated change which will make this issue stall.

dimaro’s picture

@cilefen Ok no problem :)
Returns the modified array to its original state?
Thanks.

cilefen’s picture

Status: Needs work » Closed (duplicate)
dimaro’s picture

Really is not a duplicate issue, should be postponed, no?

cilefen’s picture

@dimaro It is postponed in the fist place because this patch cannot be committed in the 8.0.x beta cycle. And, it has been decided that a single big patch in the parent issue will do it, after 8.0.x is released.