Mechanism to disable preprocessing of base fields in taxonomy and aggregator entity types so they can be configured via the field UI

Are you sure we don't want to get this done consistently in D8, at least until LTS?
This may be some work, but once #2923701: Mechanism to disable preprocessing of node base fields so they can be configured via the field UI is committed, it's mainly copy/paste, add/fix tests, fix themes.

Per the IS it is definitely doable in D8, but @AdamPS is not planning to do it as part of his one man initiative here, and no one else has shown much interest; whereas it's straightforward for D9.

I don't think that nobody would be interested. I rather think everybody's waiting for #2923701: Mechanism to disable preprocessing of node base fields so they can be configured via the field UI to finally arrive, so we can fix it for all other entities consistently, 1:1. If in the end we should be missing the D8 LTS target (which I'm not excpecting), then we can still move on to D9.

@AdamPS
Sure, as soon as I’m back from vacations, I will look into what’s missing plus do a first code review.

I’m not assigning as everyone who gets to it earlier, is welcome!

Currently working on the tests.

Here are the tests. Please review whether everything works the way it was meant to be.

Here is an only tests version of your patch to confirm the tests fail without the patch

Good idea. Indeed I wasn't 100% sure we're getting the title/name fields right, neither am I on #2923701: Mechanism to disable preprocessing of node base fields so they can be configured via the field UI which hardly tests the title field at all.

I was also unsure whether we're allowed to combine the three quite similar helper modules aggregator_feed_display_configurable_test and taxonomy_term_display_configurable_test with node_display_configurable_test into a single one. Finally, we opted for a single 'enable_base_field_custom_preprocess_skipping' flag.

Needs work for tests and CR. If we are quick and get this into D8.7 then can share the same CR as the node one.

I guess we're too late for D8.7, right?

While the tests should be properly testing the impact of the toggle, I'm a bit disappointed to see the tests aren't specific enough. :/

TermDisplayConfigurableTest fails in line 55 with an exception, but shouldn't it fail on the assertions in line 48 or 49?
Even worse, FeedDisplayConfigurableTest doesn't fail at all. Or should it?

I need to figure out better how we're expecting things to be today vs. after the patch. Finally, out-of-the-box behavior shouldn't change at all.

For the tests, will have to test a teaser view. On the entity own page, the entity label gets stolen by the title block.

Ouch, I missed that hint. Indeed that is what we should be doing. Reassigning to rework the tests. I will also take a look at the other two issues.

I edited the CR very slightly for clarity. Fix & tests look good to me.

I think the code change itself is fine. I don't think this patch makes it clear enough that users are expected to make changes before Drupal 9. The patch doesn't mark anything deprecated and doesn't trigger any warnings.

@lauriii Thanks for the feedback.

I don't think this patch makes it clear enough that users are expected to make changes before Drupal 9. The patch doesn't mark anything deprecated and doesn't trigger any warnings.

The only change the patch expects users to make before D9 is explained in the CR in this way:

The special variables representing the preprocessed base fields are now optional in templates. ... Contrib theme developers should make the corresponding changes to their templates so that they work correctly if configurable display of base fields is enabled. However the changes are fully back-compatible, so that existing sites that do not enable configurable display of base fields do not need to change templates.

However, I'm guessing this is NOT you're concern. I'm imagining you're thinking that maybe we want to remove these special variables altogether in D9, and you're concerned that we're not properly warning developers about this.

This is a relevant concern, as ultimately we are building towards #3015623: [PP-many] Remove outdated code relating to "Expose Title in Manage Display". The meta issue #2353867: [META] Expose Title and other base fields in Manage Display explains the sequence of steps necessary to get that stage.

The key point for this issue though is that:

1) This issue does nothing for taxonomy and aggregate that has not already been done for node in #2923701: Mechanism to disable preprocessing of node base fields so they can be configured via the field UI; this issue simples makes core intenally consistent, and this is worth doing now even if the solution already committed for node is imperfect.

2) If we need to find a way to deprecate or warn about the use of (now optional) variables being supplied to templates, we should create a follow-up issue to figure out how to do that and apply it not only to taxonomy and aggregate but also to node. This is a similar approach to that you recently requested in #2993647: Correctly determine when to display fields as inline where you asked for a follow-up issue to establish how to deprecate templates.

Therefore I'm tentatively suggesting this should be be RTBC; if you concur I will create 2 follow-ups: one to establish a policy on deprecating variables supplied to templates, one to implement it for these base fields.

The patch code looks good to me, aside from a few non-blocking issues (see review below), however I was expecting to see more entity types covered by this patch.

Specifically I was expecting:

• aggregator item
• block content (although I can imagine this might be tricky)
• comment

Can we at least update the IS to explain the rationale behind not addressing those? Given that we have the node part of this already in and that this has a BC-layer in place, I think it should be possible to commit the patch during the alpha window, but we need to make sure core is in a consistent state once beta lands.

For this reason we should either document why it's ok not to convert those entity types or update the patch to handle them.

1. +++ b/core/modules/aggregator/aggregator.theme.inc
   @@ -34,6 +34,14 @@ function template_preprocess_aggregator_item(&$variables) {
   + * By default this function performs special preprocessing to create a separate
   + * for the title base field. This preprocessing is skipped if:
   

   separate what? :)

2. +++ b/core/modules/aggregator/aggregator.theme.inc
   @@ -34,6 +34,14 @@ function template_preprocess_aggregator_item(&$variables) {
   + *   hook_entity_type_build().
   
   +++ b/core/modules/taxonomy/taxonomy.module
   @@ -244,6 +244,15 @@ function taxonomy_theme_suggestions_taxonomy_term(array $variables) {
   + *   hook_entity_type_build().
   

   hook_entity_type_build() should be used to provide new entity type definitions, not to alter existing ones. For that we have hook_entity_type_alter(). We need to update the CRs (also the node one) to reflect this.

3. +++ b/core/modules/aggregator/tests/modules/aggregator_feed_display_configurable_test/aggregator_feed_display_configurable_test.module
   @@ -0,0 +1,27 @@
   +  if ($entity_type->id() == 'aggregator_feed') {
   
   +++ b/core/modules/taxonomy/tests/modules/taxonomy_term_display_configurable_test/taxonomy_term_display_configurable_test.module
   @@ -0,0 +1,27 @@
   +  if ($entity_type->id() == 'taxonomy_term') {
   

   ===

4. +++ b/core/modules/aggregator/tests/src/Functional/FeedDisplayConfigurableTest.php
   @@ -0,0 +1,56 @@
   + * @group node
   

   Wrong group

5. +++ b/core/modules/aggregator/tests/src/Functional/FeedDisplayConfigurableTest.php
   @@ -0,0 +1,56 @@
   +    $assert->elementNotExists('css', 'div.field--name-title');
   
   +++ b/core/modules/taxonomy/tests/src/Functional/Views/TermDisplayConfigurableTest.php
   @@ -0,0 +1,59 @@
   +    $assert->elementNotExists('css', 'div.field--name-name');
   

   I'd add an assertion in both cases that the label itself does not exists in the page.

6. +++ b/core/modules/taxonomy/tests/src/Functional/Views/TermDisplayConfigurableTest.php
   @@ -0,0 +1,59 @@
   +use Drupal\Core\Entity\Entity\EntityViewDisplay;
   

   Unused

7. +++ b/core/modules/taxonomy/tests/src/Functional/Views/TermDisplayConfigurableTest.php
   @@ -0,0 +1,59 @@
   +/**
   + * Tests making aggregator_feed base fields' displays configurable.
   + *
   + * @group node
   + */
   +class TermDisplayConfigurableTest extends TaxonomyTestBase {
   

   Wrong PHP doc and @group.

It's ok to address the other modules in separate issues, let's hope we can get that ready before beta as well, thanks!

Heroic work, Adam.

+++ b/core/modules/aggregator/aggregator.module
@@ -121,16 +119,45 @@ function aggregator_entity_extra_field_info() {
+      'weight' => 2,

Weight 2 is used for image above; not sure if this matters.

Ah, of course.

I agree with @lauriii that we need to find a way to mark the stuff that is meant to be removed in #3015623: [PP-many] Remove outdated code relating to "Expose Title in Manage Display" as deprecated and warn users about it. I don't think it will be possible to remove from D9 code that was not deprecated in 8.8, worst case it'll happen in D10.

Anyway, given that the corresponding node changes are already committed, I don't think we have good reasons to hold up this improvement. The latest patch looks good to me, thanks!

Applied the following diff and committed/pushed 9451d02 to 9.0.x and friends. Thanks!

diff --git a/core/modules/aggregator/tests/src/Functional/AggregatorDisplayConfigurableTest.php b/core/modules/aggregator/tests/src/Functional/AggregatorDisplayConfigurableTest.php
index bd8f1263a7..0a2dcb652b 100644
--- a/core/modules/aggregator/tests/src/Functional/AggregatorDisplayConfigurableTest.php
+++ b/core/modules/aggregator/tests/src/Functional/AggregatorDisplayConfigurableTest.php
@@ -13,6 +13,9 @@
  */
 class AggregatorDisplayConfigurableTest extends AggregatorTestBase {
 
+  /**
+   * {@inheritdoc}
+   */
   protected function setUp() {
     parent::setUp();

Published the CR.

Please let's add a release notes snippet to the IS.

AFAICT this broke 9.0.x HEAD: https://www.drupal.org/pift-ci-job/1461889, all my recent patches are failing. Ah … literally 4 minutes ago, https://git.drupalcode.org/project/drupal/commit/49891d0 was pushed to fix that 🥳

I … don't know how I managed to credit Alex Pott without clicking anything. I guess I cross-posted?

The commits in #74 have removed Core: 8.x from the new test module's .info.yml. So now when I run a test for my contrib module at core 8.9 on Drupal.org or Travis, every test fails with

Drupal\Core\Extension\InfoParserException: The 'core' or the 'core_version_requirement' key must be present in 
core/modules/aggregator/tests/modules/aggregator_display_configurable_test/aggregator_display_configurable_test.info.yml

I was alerted to this by Travis cron run an hour ago. See manual test run https://www.drupal.org/pift-ci-job/1462019

This is caused by the check in InfoParserDynamic parse()
https://git.drupalcode.org/project/drupal/blob/8.9.x/core/lib/Drupal/Cor...

Could you explain the reason for deleting the "core 8.x' line? Currently all my contrib testing at 8.9 is broken and I don't think my module is unique in this. Hopefully there is a good solution. Thanks.

Jonathan

@jonathan1055:

Thanks for the heads up, I think the previous fix was meant to be applied only to 9.0.x. I just reverted it on 8.9.x, which should start working again now.

Thanks @plach, yes it did look like an incorrect deletion of that line. I have just re-run the 8.9 tests on Drupal.org and Travis, and all is back to normal, all passing. Auto e-mail alerts for test failures are a great thing.

Jonathan

Checked with @plach; this change is non-disruptive and therefore more of a release highlight.