Deprecate hide() and show()

Looks good to me.

This is fairly pedantic, but in most cases OO functions are "Capitalized::thing()" (only exceptions being internal objects like self:: or parent::) so it's weird in this instance to see "element" vs. "Element." Could we get a quick re-roll to fix that?

We need an issue summary to explain at least why we are doing this.

Reroll for #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4. However, we still need that issue summary update.

Is this still a thing?

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21... Element still doesn't have a show() or hide(), but it was a year ago.

Needs a reroll.

$ git apply 2258355-psr4-reroll.patch 
error: patch failed: core/includes/common.inc:3512
error: core/includes/common.inc: patch does not apply
error: patch failed: core/lib/Drupal/Core/Render/Element.php:162
error: core/lib/Drupal/Core/Render/Element.php: patch does not apply
error: patch failed: core/modules/comment/src/Plugin/views/row/Rss.php:8
error: core/modules/comment/src/Plugin/views/row/Rss.php: patch does not apply
error: patch failed: core/modules/node/src/Plugin/views/row/Rss.php:8
error: core/modules/node/src/Plugin/views/row/Rss.php: patch does not apply
error: patch failed: core/modules/views_ui/views_ui.theme.inc:172
error: core/modules/views_ui/views_ui.theme.inc: patch does not apply

Rerolled the patch successfully as per the latest release.

Recover method isEmpty i Drupal\Core\Render\Element.
Corrected reference to Drupal\views\Plugin\views\row\RssPluginBase.

Replace drupal_render($element, TRUE) by \Drupal::service('renderer')->render($element) in render().

Rerolled the patch.

At this point we probably need to just deprecate this wrappers.
Patch should just replace usage of old functions with new ones and deprecate them

+++ b/core/modules/comment/src/Plugin/views/row/Rss.php
@@ -106,15 +107,21 @@ public function render($row) {
-    $build = comment_view($comment, 'rss');
+    $build = \Drupal::entityManager()->getViewBuilder('comment')->view($comment, 'rss');
...
+    // Hide the links if desired.
+    if (!$this->options['links']) {
+      Element::hide($build['links']);
...
-      // We render comment contents.
+      // We render comment contents and force links to be last.
+      $build['links']['#weight'] = 1000;

+++ b/core/modules/node/src/Plugin/views/row/Rss.php
@@ -152,6 +151,11 @@ public function render($row) {
+    // Hide the links if desired.
+    if (!$this->options['links']) {
+      Element::hide($build['links']);

looks unneeded changes

There's separate issue to manage "position of comment links"

Removed the modifications made to /core/modules/comment/src/Plugin/views/row/Rss.php and /core/modules/node/src/Plugin/views/row/Rss.php

1. +++ b/core/includes/common.inc
   @@ -1104,19 +1104,17 @@ function drupal_render_children(&$element, $children_keys = NULL) {
   + * element is shown with Element::show() before rendering, so it will always be
   + * rendered even if Element::hide() had been previously used on it.
   
   @@ -1145,22 +1143,23 @@ function render(&$element) {
   + * element, be sure to call Element::hide() on the element before its parent
   
   @@ -1174,22 +1173,23 @@ function hide(&$element) {
   + * element, be sure to call Element::show() on the element before its parent
   

   "Element" should fully qualified class pass

2. +++ b/core/includes/common.inc
   @@ -1104,19 +1104,17 @@ function drupal_render_children(&$element, $children_keys = NULL) {
   - * @see \Drupal\Core\Render\RendererInterface
   ...
   + * @see drupal_render()
   

   not sure this change, interface more descriptive

3. +++ b/core/includes/common.inc
   @@ -1145,22 +1143,23 @@ function render(&$element) {
   + * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.
   
   @@ -1174,22 +1173,23 @@ function hide(&$element) {
   + * @deprecated in Drupal 8.x-dev, will be removed before Drupal 8.0.
   

   @xjm probably 9.x?

Element still doesn't have methods 'show()' and 'hide()'.
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21...

I tried to make reroll patch for 8.6.x


Needs work for 9.3.x and collision with #2794261: Deprecate render() function in common.inc

Re-rolled patch for 9.3.x. Please review

The deprecation message needs updating to mention 9.3.x as the deprecation version and to match our standards, see https://www.drupal.org/pift-ci-job/2159191 for the codesniffer output.

1. +++ b/core/includes/common.inc
   @@ -376,23 +377,21 @@ function drupal_attach_tabledrag(&$element, array $options) {
   - * @param $element
   + * @param array $element
   ...
   - * @return
   + * @return array
   ...
   - * @see \Drupal\Core\Render\RendererInterface
   - * @see show()
   - * @see hide()
   + * @see drupal_render()
   

   this changes are unrelated to deprecation

2. +++ b/core/includes/common.inc
   @@ -422,23 +421,25 @@ function render(&$element) {
   + * @deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. Use
   
   @@ -452,23 +453,25 @@ function hide(&$element) {
   + * @deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. Use
   

   it should point to 9.4.0

3. +++ b/core/lib/Drupal/Core/Render/Element.php
   @@ -200,4 +200,53 @@ public static function isEmpty(array $elements) {
   +  public static function hide(&$element) {
   ...
   +  public static function show(&$element) {
   

   needs deprecation test and new issue to remove deprecated functions like #3260778: Remove deprecated code from bootstrap.inc

Points 1,2,3 from previous comment have been fixed.

Thanks for such a quick review!

Thank you! looks mostly ready

+++ b/core/includes/common.inc
@@ -432,13 +433,17 @@ function render(&$element) {
+  @trigger_error('hide() function is deprecated in drupal:9.4.0 and is removed from drupal:10.0.0. Use \Drupal\Core\Render\Element::hide() instead.', E_USER_DEPRECATED);

@@ -462,13 +467,17 @@ function hide(&$element) {
+  @trigger_error('show() function is deprecated in drupal:9.4.0 and is removed from drupal:10.0.0. Use \Drupal\Core\Render\Element::show() instead.', E_USER_DEPRECATED);

+++ b/core/tests/Drupal/Tests/Core/Render/Element/ElementDeprecationTest.php
@@ -0,0 +1,41 @@
+    $this->expectDeprecation('show() function is deprecated in drupal:9.4.0 and is removed from drupal:10.0.0. Use \Drupal\Core\Render\Element::show() instead.');
...
+    $this->expectDeprecation('hide() function is deprecated in drupal:9.4.0 and is removed from drupal:10.0.0. Use \Drupal\Core\Render\Element::hide() instead.');

Deprecation messages should include link to https://www.drupal.org/node/3261271

So when it will be found in logs users can navigate to the link

Details could be found at https://www.drupal.org/about/core/policies/core-change-policies/drupal-d...

Done. Hope this will be the last iteration :)

Yay! looks ready for commiters' eyes

Bump, 3 weeks - no response. Patch still applying over 9.4.x branch.

Wrong status

The change record is empty, so is the issue summary.. Would be great if someone could update those with necessary information.

Updated Functions hide() and show() are deprecated.

Patch or MR doesn't apply anymore
The last patch or MR doesn't apply to the target branch, please reroll the code so that it can be reviewed by the automated testbot.

D10 version needed
At this time we would need a D10.1.x patch or MR for this issue.

Also the issue summary could still use some text, at least a few sentences.

Random test failure, ordered retest.
Also updated CR to 10.1.x/10.1.0.

Updated IS.

Test should make sure that functions returns same result

Thanks @andypost, tests changed.

Thank you!

Added some comments on the MR. Also if we are changing the documentation to show and hide - then we need to re-flow it to 80 chars. There are some short lines now.

Addressed review comments by @alexpott

One PHPCS error

Fixed PHPCS error.

Thank you! looks ready again

I left some comments are the MR that shoukd help move forward.

I added several suggestions on the MR to improve the new APIs, the deprecations, and the documentation.

If you're unable to accept suggestions in the UI even with push access, respond to the thread with 👍 and I or the original PR author can accept the suggestions on your behalf.

Thanks everyone!

@xjm
Thanks for the review and suggestions.
I have applied the suggests. Ready for review again.

Added few suggestions

@voleger Thanks. Committed them.

Merged HEAD so that retesting will work.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rerolled against 10.1.x branch. Addressed last review comment.

Hi @volger, Reviewed your patch on drupal version -10.1.x.It was successfully applied and after the patch both methods moved to class \Drupal\Core\Render\Element. Adding Screenshots for the reference . Need RTBC +1

Reviewing MR 3011

Deprecation tests are there
Change record makes sense and offers the alternative to use.
The 1 open thread has been resolved by renaming to ProceduralApiDeprecationTest

Think this is good to go.

Couple of minor comments on the MR.

Fixed both nits

Seems comments in the MR have addressed so moving to RTBC.

I am wondering now if we should just deprecate these with no replacement. These were first introduced in #455844: Allow more granular theming of drupal_render'ed elements to simplify life for PHPTemplate themers so they could exclude parts of render arrays directly from a template.

Now we are using Twig, I am not sure we need these at all? We have the without() filter in Twig to serve this purpose already. template_preprocess_file_widget_multiple() could be refactored so it doesn't use them. I can't find any uses in contrib either - http://grep.xnddx.ru/search?text=hide%28&filename= only finds false positives in JavaScript files.

Good point by @longwave.
Should we keep methods for backwards compatibility even if not in use?

Pushed commit which inlining usage without replacement, moved documentation to \Drupal\Core\Render\RendererInterface::render() as it still explains how elements and children are traversed using #printed semaphore.

CR needs update and +1 to deprecate without replacement

To prevent collisions with @VladimirAus here's a patch I suppose, maybe it needs better wording instead of #printed=TRUE in deprecation message

Tagging for FEFM review.

Updated IS and CR

Re-roll after #2602326: Unused local variable $key (at line 49) in file.field.inc

unrelated failures in head

All code changes look good to me.
Deprecation message testing has been added.
For me it is RTBC.

known failure Drupal\Tests\ckeditor5\FunctionalJavascript\MediaLibraryTest

Looks good, just some nits on the comments and deprecation:

1. +++ b/core/includes/common.inc
   @@ -348,14 +348,8 @@ function drupal_attach_tabledrag(&$element, array $options) {
   + * Refer to Drupal\core\lib\Drupal\Core\Render\RendererInterface::render()
   + * for additional documentation.
   

   "Drupal\core\lib" is incorrect, and not even sure we need this comment, given we are removing it anyway and there is a change record. This needs changing in two places.

2. +++ b/core/includes/common.inc
   @@ -363,11 +357,14 @@ function drupal_attach_tabledrag(&$element, array $options) {
   +  @trigger_error('The global hide() function is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. Use to inline #printed=TRUE instead. See https://www.drupal.org/node/3261271', E_USER_DEPRECATED);
   

   "Set the #printed element property to TRUE instead." reads better to me. This needs changing in all places that refer to the deprecation.

3. +++ b/core/lib/Drupal/Core/Render/RendererInterface.php
   @@ -116,6 +116,15 @@ public function renderPlaceholder($placeholder, array $elements);
   +   * element, be sure to set element['#printed']=TRUE before its parent
   

   Let's use real code: $element['#printed'] = TRUE

Fixed, I find the comment useless as well, also there's a fixed @see links to render method docs

Seems to have build failures.

But checked interdiff and points #109 appear to be addressed.

It was caused by reverted #3268809: Fix class comment doc blocks in tests for 'Drupal.Commenting.DocComment.ShortSingleLine'

Requeued

Deprecating these seems worthwhile. I'd like to see a little bit of additional docs just to make it easier to grok if someone is dealing with code that now uses #printed instead of hide() / show(), which may not be the most consequential thing but a relatively simple change.

1. Lets also update the doc for #printed in core/lib/Drupal/Core/Render/Element/RenderElement.php to clarify that it can be set to TRUE to prevent rendering as well, similar to what was added in RendererInterface

2. +++ b/core/modules/file/file.field.inc
   @@ -66,9 +66,9 @@ function template_preprocess_file_widget_multiple(&$variables) {
        // Delay rendering of the "Display" option and the weight selector, so that
   

   Since changing #printed is a little less self-explanatory than a hide() method, could this comment be expanded slightly to describe how the rendering is being delayed?

FWIW these methods are used by contrib and probably custom - see
http://grep.xnddx.ru/search?text=+hide%28&filename=.php
http://grep.xnddx.ru/search?text=+hide%28&filename=.module
http://grep.xnddx.ru/search?text=+show%28&filename=.module

There's loads and in popular modules. Are we really sure about this?

Looking at that list saml and editorally would be disruptive.

This issue summary claims that there are no contrib usages - this in correct as per #114 - I'm not really sure how to update it.

Apologies for the mistake in #95, I should have filtered by extension. I went through the lists in #114 and while there are a handful of genuine uses, some are form elements that should probably be hidden by setting #access instead, and others are false positives:

• accessibility: a comment in a PHPTemplate template
• chinese_address: no published 8.x release
• zen: an unported PHPTemplate template
• bricks: affected, but probably should use #access instead
• editoria11y: false positive in a method name
• epub_reader_framework: affected, again should probably use #access as this is hiding a form element
• faq: affected
• field_group_background_image: affected
• fpa: affected
• media_acquiadam: affected, but should probably use #access
• optimizedb: false positive in a method name
• site_settings: affected, but should probably use #access
• ui_patterns_settings: affected
• packery: affected, but should use #access
• damopen: affected, but should use #access
• flickity: : affected, but should use #access
• protected_file: affected (looks similar to the core preprocess that we have to alter here)
• saml_sp: affected, but should use #access
• uber_publisher_social_post: affected, but should use #access
• vertical_tabs_config: affected
• oembed: no published 8.x release
• module_filter: false positive in JavaScript

As it looks like the many of the cases in contrib are actually using this to hide form elements, while this works, it's not really how it was intended to be done as far as I am aware - form elements should set #access to FALSE instead. If this is correct, to me this is a good reason to deprecate this and point users in the correct direction.

I think the options are:

1. Do nothing, leave this alone
2. Deprecate, explain in the deprecation/change record that form alters should use #access and render elements should use #printed
3. Move hide() and show() to static methods on Element

I think it just needs better documentation for #printed as #113 said and deprecation

Great points about mixing it with #access but usage in contrib tells that no reason to keep it as public API