Deprecate hide() and show()

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 🐛 Unused local variable $key (at line 49) in file.field.inc Fixed

The last submitted patch, 103: 2258355-103.patch, failed testing. View results →

unrelated failures in head

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

The last submitted patch, 103: 2258355-103.patch, failed testing. View results →

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 📌 Fix class comment doc blocks in tests for 'Drupal.Commenting.DocComment.ShortSingleLine' Fixed

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

So to me it looks like the majority of cases in contrib are actually using this to hide form elements; while this works it's not really how it was intended to be used as far as I am aware.

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