Views status field triggers a PHP 8.5 deprecation on /admin/people

Seems this came over from the gin move over to core? Either way the error does go away with the change.

This was added to fix a bug - see #3502789: The "Unpublished" badge for an unpublished translation is green - yes it is causing problems elsewhere and perhaps is not the correct fix but we should cause this.

See also #3576477: Langcode not correctly detected in field--status which leads to PHP8.5 deprecation issue which is the same issue just for gin.

I think we need a different solution to the original problem in #3502789: The "Unpublished" badge for an unpublished translation is green

My suggestion would be that the theme shouldn't work what is apparently bug/inconsistency between views/entity translations. The whole template seems a bit problematic. It comes with a hardcoded assumption that the published status field is called "status" and that the entity for it does have an isPublished() method.

I'd suggest creating a new bug report in core about that, investigate what exactly is going on and work on a better resolution of this?

@berdir I agree - I think we should remove core/themes/default_admin/templates/views/views-view-field--status.html.twig entirely and open an issue to add it back fixing the multilingual issue in views. The entity should be in the language that the row used but it might be tricky to change. And definitely we should not be working around this in the theme layer.

I was also thinking that removing this entirely is something to consider. We would lose the formatting of the status completely though? I'm personally fine with that. removing a feature in default_admin that existed in gin in contrib isn't a regression, but I don't know who exactly needs to make that decision.

Views already has formatting options to display this as Yes/No, a checkbox, or something else, a "Badge" could be another option there?

Also, we have this "entity status badge" thing as a component in the navigation module for the top bar. This is essentially the same, could be reused if we push it up somewhere (where though...)

In addition to the Twig template fix, I'd suggest also hardening `hasTranslation()` in `ContentEntityBase` to guard against null langcodes:

In `/core/lib/Drupal/Core/Entity/ContentEntityBase.php`, add a null coalescing assignment at the start of `hasTranslation()`:

$langcode ??= '';

This would make the method more defensive and prevent the deprecation from surfacing even if a null langcode is passed from other places in the future — complementing whatever fix is applied at the template or views layer.

FWIW, the same warning is also displayed on /admin/content/files page, but will disappear on a second page load (until caches are cleared again).

Updated the issue title to more accurately pinpoint the problem

The merge request now removes the problematic views-view-field--status.html.twig template and its associated styling per #12 / #13. I created a followup issue to implement entity status formatting for views fields, see #3608223: Add entity status formatting to views field

The Proposed resolution is now to remove the twig template instead of updating it

Removing this entirely may be problematic, it has been one of those feature that made Gin to be attractive and would be a sort of regression for DCMS who will be switching over when it's no longer experimental.

In Gin, we have MRs 758, 785, and 788, but haven't decided yet which path to follow.

For default_admin following the status badge from the navigation module seems to be the most logical approach.

I think this is relatively urgent and if no proper fix can be decided on soon, at the very least this file should be removed for the time being with the next GIN update. As I commented under #3580668, a temporary fix is to delete the "themes/gin/templates/views/views-view-field--status.html.twig" but we have dozens of sites and because this error usually occurs within a loop, all errors logs are completely inundated with this error. I've already manually deleted a file for the "worst offending" sites, but a GIN update without a fix for this would make this error reappear, of course. So this could quickly turn into a very unpleasant whack-a-mole I'm afraid - so that's why I'd suggest removing it until a proper fix is decided on.

@rgpublic this is not the right issue to discuss Gin topics. You're welcome to contribute to the process over in the Gin issue queue to find a conclusion of how to process with the 3 MRs that I linked to above. The matching issues are #3576477: Langcode not correctly detected in field--status which leads to PHP8.5 deprecation issue, #3583491: Using null as an array offset is deprecated error on admin/content and admin/people, and #3591598: row.node_field_data_langcode in views-view-field--status.html.twig may be null. And if you want to make your described workaround persistent, you can add the command to delete a file after each update as a post update script in your composer.json.

I've opened a new MR!16557 with a proper implementation of the status field twig template for views, instead of removing the entire feature. I've also added tests, for those I've used the help of Opus 5, but reviewed them carefully to make sure they're all reasonable.

What the MR does is to provide a preprocess hook for the status field in views. It provides a boolean and nullable variable that contains the information if the entity contained in the views row is published or not. Should the row not contain an entity, or an entity that has no published key, then that variable is set to NULL. The twig template can then be simplified to only to its magic with the status marker if that variable is either TRUE or FALSE.

The Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Rebased the MR.

The code looks good to go but not sure about BC policy for twig templates

@alexpott the only twig file being changed is in default_admin, same for the new hook. And that theme doesn't have any BC commitments, at least not at this point, I'd say.

For twig templates I've always taken the stance they need a CR. If you maintain a contrib theme it's the only way I know to get that message out that a template changed.

@smustgrave even if the theme is still experimental and the change is just a bug fix that behaves like before but avoids a warning under certain circumstances?

The twig template and preprocess work and make sense, in general.

However, I propose the following changes:

1. Create a separate getResultRowEntity()
   

   $relationship_id = $field->options['relationship'] ?? 'none';
       $entity = $relationship_id === 'none'
         ? $row->_entity
         : ($row->_relationship_entities[$relationship_id] ?? NULL);

   The preceding code is also used in FieldPluginBase::getEntity(), although with a slightly different syntax. To change FieldPluginBase::getEntity() is out of scope of this issue, but could be done later. Anyway, a separate function would meet Single Responsibility Principle and the function could be later reused in FieldPluginBase or elsewhere.

2. Move ($entity->getEntityType()->getKey('published') !== ($field->definition['field_name'] ?? NULL)) to its own function. Creating isPublishedStatusField(EntityField $field, EntityPublishedInterface $entity): bool reads better as a named predicate.
3. The patch considers publishable entities whose field for the published status has a different name. It skips the highlighting for such fields. Add a note to isPublishedStatusField() making the intentional skip explicit.
4. The code doesn't check if $variables['published'] is already set. If that is intentional, it should be stated. Otherwise add a check.
5. Maybe move the entire hook code to its own function computePublishedState(): ?bool and add unit tests.

The explanatory comments (why we avoid getEntity(), why we check the key) don't disappear entirely. They move onto the helper doc blocks.

Great suggestions, thanks @boromino. I've applied those to the current MR, please have another look.

Code looks good and works, tests pass.

I'm kind of OK with whatever quick fixes we want to add to get rid of the PHP deprecation messages. I'm not so sure about the field map based approach in the most recent MR, I think there's an ongoing discussion on whether or not we want to keep that field map at all. It's expensive to build on a cold cache for one thing, but also, that's just checking if it's boolean. I think the published entity key in the previous MR was deliberate, we do not want to display "Published" on something that's not actually a published key.

I still think we should explore as a follow-up what I wrote in #14. Views has configurable formatter that can display a boolean field as Yes/No, checkmarks and other options, see \Drupal\Core\Field\Plugin\Field\FieldFormatter\BooleanFormatter::getOutputFormats(). IMHO, this kind of output shouldn't be some hardcoded magic in a template, it should be something that's explicitly configured for these views. Gin didn't have that option, default_admin however does since it is in core.

Had a couple thoughts on MR 16557:

• The user entity type does not implement EntityPublishedInterface nor does it have a published key, so it looks like this:
• The preprocess hook code seemed more complex than it needed to be

On one hand, the issue with user could be fixed in a separate issue to make the entity type implement the interface and add the key, but that won't handle other entity types that are similar. And my thinking is that a status field that has boolean values is going to represent the published status a great majority of the time. So originally, I refactored the preprocess a bit to check whether the field had only boolean-like values, but then I realized that could just be done in the template.

The template only alternative approach is MR 16769.

Though now I prefer the template only alternative, I finished reworking the preprocess approach, so that instead of checking for boolean values, it checks the field map to see if the field is a boolean. This alternative is MR 16768. This handles edge cases better than the template only approach does, but it does have drawbacks as mentioned in #40.

Either of these alternatives handle the user status field correctly:

I still think we should explore as a follow-up what I wrote in #14. Views has configurable formatter that can display a boolean field as Yes/No, checkmarks and other options, see \Drupal\Core\Field\Plugin\Field\FieldFormatter\BooleanFormatter::getOutputFormats(). IMHO, this kind of output shouldn't be some hardcoded magic in a template, it should be something that's explicitly configured for these views.

I agree with this, though it would mean reconfiguring some existing admin views.

For now, I think the template-only approach in MR 16769 is the minimal fix to address the PHP warning without regression.

I do wonder if the new kernel test might be more extensive than needed, but I've exhausted the brain power I have for today on this.

> For now, I think the template-only approach in MR 16768 is the minimal fix to address the PHP warning without regression.

I think it's questionable what exactly the expected and correct behavior is. Your screenshot doesn't replicate HEAD, it actually goes further and fixes it, on HEAD file and user views are not using green pills, they are always grey. I think both the template fix and your other MR now make those green too. Which I guess is fine, but I'd rather not have pills than pills that never change their color.

I'm not sure if it should be limited to published entities or not, it kind of makes sense on user and file, agreed. They don't use a published key on purpose, as users/files aren't published, it means something else for them. You use the field map to avoid the get entity part, but we have an existing API for thata on \Drupal\views\Plugin\views\field\FieldPluginBase::getEntity, which is the base class of EntityField, so we can just call that? And then we can check the field type on that and avoid the field map at runtime.

I'm honestly fine with any of those version as long as we clean it up and avoid performance issues, if we create a follow-up issue to move this into views. One reason for that is that this is still hardcoded for a field named "status". It wouldn't work on \Drupal\menu_link_content\Entity\MenuLinkContent for example, which uses "enabled" for it's published key (we don't really have a use case for MenuLinkContent views, it's just an example)

I've tested both MRs from @godotislate and can confirm that they both work. MR!16768 in particular does use the pills and they correctly switch color both on content and user on main. That would be my preference moving forward for the quick-fix.

Other than that, @berdir's suggestion in #14 should be the proper solution, i.e. delegating this to a views-field formatter, updating the views for new installations, and removing the field template from default_admin.

I leave this on NW as I'm uncertain what's still open, @berdir?

If the result of MR 16768 is preferred then my suggestion is to to that without the field map, using the existing getEntity() method that we can use and then checking the type through the field definitions.

I had a typo in previous comment, so correcting here:

MR 16768 uses preprocess with the field map. The reason it use the field map is that in the original MR 16557, there is a comment about why getEntity() is essentially reimplemented as getResultRowEntity() in the Hooks class, because getEntity() can trigger log messages if the row does not have an entity. Though EntityField::getValue() actually calls getEntity() anyway, so I don't think we need getResultRowEntity() (or the field map) at all. If MR 16768 is preferred, I can make the requested changes.

MR 16769 is the template-only approach. No preprocessing required.

Just wanted to confirm which one we want to go forward with. Both 16768 and 16769 display the green/gray pills correctly for users and files. (I prefer the template-only approach, but I'm fine with either.)

I care about not using getFieldMap() for now, see #3565128: Avoid calls to EntityFieldManager::getFieldMap() where unnecessary, other than that, no strong opinion. No idea if the template only approach would trigger on something that is unexpected, in theory it could, but it probably doesn't hurt too much and we can always point to the to-be-created follow-up if someone doesn't like it.

And yes, something else would almost certainly trigger those log messages anyway, it's fallback and should not happen on regular cases (it can happen with inconsistent data and race conditions when entities are being deleted while the view is rendered, I've seen that load tests for example.

OK, removed field map from https://git.drupalcode.org/project/drupal/-/merge_requests/16768.

Maybe I'm confusing myself just now, but I was looking back to the original template-only code and got reminded, that the root cause was that the template tried to optionally load the translated entity before determining the publishing status:

{% set entity = row._entity %}
{% if entity.hasTranslation(row.node_field_data_langcode) %}
  {% set entity = row._entity.getTranslation(row.node_field_data_langcode) %}
{% endif %}

And that failed for views that haven't been node-based. Are we missing that functionality by now? It seems like we do not bother about that translated entity any longer.

EntityField::getValue() gets the translated entity. In the template, the field variable is the field handler, and row is the result row, so calling getValue() gets the correct translated value if status is an entity field.

The test coverage for it was also copied from the original MR: https://git.drupalcode.org/project/drupal/-/merge_requests/16769/diffs?f...

Thanks @godotislate, looks like I stopped seeing the forest for all the trees.

I've now reviewed both your MRs again, and tested them on latest main. All looks great and works as described. For some reason I still prefer MR!16768, but I'd be OK with MR!16769 too.

Fair enough, let's go forward with MR!16768. Closing MR!16769.

Adding the follow up tag so it's not forgotten.

Created #3619121: Support colored status badge in boolean formatter instead of only in default_admin

Removing admin theme from the title because this is in the Admin theme component

This looks like a pragmatic solution and we've opened follow ups to address this in a better way in the future.

Committed and pushed 51ff2c2fe01 to main and fb9137bc7e8 to 11.x and e2a8f121795 to 11.4.x. Thanks!