Problem/Motivation

EntityDisplayBase::handleHiddenType is a method added in as a BC layer and marked deprecated despite being needed and used in Drupal 8 core.

Proposed resolution

Properly trigger an error when the BC layer is activated so contrib knows to not use type => hidden.

Duplicate the handleHiddenType() logic inline in the one place it is used, and fully deprecated ::handleHiddenType()
Add appropriate tests for all code paths.
Follow up issue is at #2799641: Remove automatic 'type' handling from EntityDisplayBase in favor of explicit 'region' handling

Remaining tasks

User interface changes

API changes

handleHiddenType will be properly deprecated

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mikelutz created an issue. See original summary.

mikelutz’s picture

This is probably more a matter of undeprecating it until drupal 9, and triggering an error if we detect a hidden type.

Let's start with that error and see how much breaks..

mikelutz’s picture

Status: Active » Needs review

Status: Needs review » Needs work
mikelutz’s picture

Title: Properly deprecate EntityDisplayBase::handleHiddenType » Properly deprecate type => 'hidden' component handling and undeprecate EntityDisplayBase::handleHiddenType until its usage can be removed in Drupal 9
Assigned: mikelutz » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
Related issues:
FileSize
6.39 KB
6.04 KB

Here's an attempt to fix the migration and update the tests. I might break more than I fix. I'm not sure how best to adjust the tests, so I could use a review on that.

hchonov’s picture

Undeprecate the specific method as it is currently in use.

What is the problem with the method still bring in use?

P.S.
Can't we actually completely ditch the hidden region in D9?

mikelutz’s picture

There is no problem with the method being in use, but code that is in use isn't deprecated. Code is deprecated after it is no longer in use. If this method were deprecated, then core tests would fail due to the trigger_error that would need to be added to the method.

I can't speak to deprecating the hidden region, I'm just trying to clean up old improperly implemented core deprecations in this issue.

hchonov’s picture

If this method were deprecated, then core tests would fail due to the trigger_error that would need to be added to the method.

I am confused. Look at the following code from \Drupal\Core\Entity\EntityManager:

  /**
   * {@inheritdoc}
   *
   * @deprecated in Drupal 8.0.0, will be removed before Drupal 9.0.0.
   *   Use \Drupal\Core\Entity\EntityTypeManagerInterface::clearCachedDefinitions()
   *   instead.
   *
   * @see https://www.drupal.org/node/2549139
   */
  public function clearCachedDefinitions() {
    @trigger_error('EntityManagerInterface::clearCachedDefinitions() is deprecated in 8.0.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\Entity\EntityTypeManagerInterface::clearCachedDefinitions() instead. See https://www.drupal.org/node/2549139.', E_USER_DEPRECATED);
mikelutz’s picture

I'm not sure what I am looking at. That is a properly deprecated function. It is not used by any core code.

Conversely, EntityDisplayBase::handleHiddenType() is extensively used. It is called every time a entity display config is saved. It was incorrectly marked as deprecated back before we quite understood how and when to deprecate things. This issue is purely about cleaning up the deprecation in preparation for Drupal 9.

hchonov’s picture

I was confused, I am sorry!

+++ b/core/lib/Drupal/Core/Entity/EntityDisplayBase.php
@@ -269,17 +269,17 @@ public function preSave(EntityStorageInterface $storage) {
+   * @todo Remove usages and deprecated this for removal in Drupal 9. Follow

There is something wrong with this sentence. What about something like this:

   * @todo Remove this method call and its method in Drupal 9.

?

mikelutz’s picture

Whoops, you are correct, that was a typo, was supposed to read * @todo Remove usages and deprecate this for removal in Drupal 9. Follow

I've adjusted the wording to resemble yours, thanks!

hchonov’s picture

But why not completely remove the method in D9?

tim.plunkett’s picture

I agree with #12. I don't understand the pushback on finishing up this deprecation as intended.

mikelutz’s picture

While the RMs and committers can and have made decisions on exceptions, I'm just going by policy. Strictly speaking, to remove this method when the D9 branch is opened, the method must:

Add @trigger_error('...', E_USER_DEPRECATED) at the top of the method. Add @deprecated to the docblock for the method.

(See https://www.drupal.org/core/deprecation#how-method)

This gives any contrib or custom code that might be extending EntityDisplayBase and calling $this->handleHiddenTypes() directly a notice that the method is being deprecated and will be removed, and gives them an opportunity to update their code, so they don't trigger a fatal error when D9 is released.

In order to add this trigger_error though, core code cannot call this method outside of legacy tests, or otherwise our core tests would fail.

What we would all (myself included) like to do is remove this method at the beginning of D9 when core no longer needs it, but our BC policy is to do the deprecation at the time when core no longer uses the code and then remove in the next major version.

One thing I can do is mark the method as @internal, this may help convince RMs that it is okay to remove without triggering an error.

In the future, I'd like to see these types of BC layer methods be marked private/final/internal so the don't end up being usable from custom code, but that is another discussion. By the strict reading of our BC policy and (and just to completely minimize disruption with custom code), this method should:

Be used as it currently is in the D8 cycle
When Drupal 9 opens, the call to the method should be removed, the method body should be emptied and replaced with a single @trigger_error
And it should be removed in Drupal 10.

mikelutz’s picture

hchonov’s picture

Status: Needs review » Needs work

Our BC policy does not cover protected methods. From https://www.drupal.org/core/d8-bc-policy :

Protected methods of a class should be assumed @internal and subject to change unless either the class or method itself are marked with @api. Drupal leaves most internal methods protected rather than private to allow for one-off customized subclasses when needed, but in most cases that "escape hatch" should not be relied upon indefinitely.

Therefore it is not needed to flag the method as internal and it is fine to remove the method in D9.

tim.plunkett’s picture

@mikelutz I'm confused. After you pinged me in Slack, we discussed the path forward on this. Why the sudden shift to leaving the method around needlessly for another major release?

mikelutz’s picture

@tim.plunkett I haven't shifted, and I'm really not trying to make a decision or determination here as to whether the method can be removed in Drupal 9, there is definitely a case to be made that it can be, but that is really a discussion for #2799641: Remove automatic 'type' handling from EntityDisplayBase in favor of explicit 'region' handling (and one I don't plan on participating in :-) )

My current mission is trying to get #3038170: Drupal core's own deprecation testing results completed before D9 is branched which is using phpstan to detect core @deprecated code that is used in core outside of legacy tests, which combined with #2959269: [meta] Core should not trigger deprecated code except in tests and during updates and #2856744: [META] Add trigger_error(..., E_USER_DEPRECATED) to deprecated code means I have a few hard rules here. If the method has @deprecated in the docblock then it needs an @trigger_error and can't be called from core. If you want to have it remain @deprecated, then I need to not call it from core code. We could do that by duplicating the logic in ::preSave(). I can code up that route if you like.

Beyond that, if you can convince the RMs to remove the method in D9 because it's internal or protected, then great! I'm really not trying to argue to keep it or not right now, I'm just trying to get the documentation and error triggering around deprecated code correct as best as I can. So if you and @hchonov can tell me how to meet the rules around deprecated code and still have this be removable in D9, I'm very happy to code it up.

mikelutz’s picture

Here's a version that duplicates the logic inline so that the method can be deprecated properly.

hchonov’s picture

Status: Needs review » Reviewed & tested by the community

Thank you.

tim.plunkett’s picture

+1 to RTBC, thanks!

mikelutz’s picture

Title: Properly deprecate type => 'hidden' component handling and undeprecate EntityDisplayBase::handleHiddenType until its usage can be removed in Drupal 9 » Properly deprecate type => 'hidden' component handling and EntityDisplayBase::handleHiddenType
Issue summary: View changes
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

The complete deprecation looks good.

Committed 5c20be3 and pushed to 8.8.x. Thanks!

diff --git a/core/modules/field_layout/tests/src/Kernel/FieldLayoutEntityDisplayTest.php b/core/modules/field_layout/tests/src/Kernel/FieldLayoutEntityDisplayTest.php
index 99ce7b9bf5..230493db80 100644
--- a/core/modules/field_layout/tests/src/Kernel/FieldLayoutEntityDisplayTest.php
+++ b/core/modules/field_layout/tests/src/Kernel/FieldLayoutEntityDisplayTest.php
@@ -32,7 +32,7 @@ public function testPreSave() {
         'name' => ['type' => 'hidden', 'region' => 'content'],
       ],
       'hidden' => [
-        'bar' => TRUE
+        'bar' => TRUE,
       ],
     ]);
 

coding standards fix on commit

  • alexpott committed 5c20be3 on 8.8.x
    Issue #3082644 by mikelutz, hchonov, tim.plunkett: Properly deprecate...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.