Multiple usages of FieldPluginBase::getEntity do not check for NULL, leading to WSOD

Conflicts with 🐛 Cannot use relationship for rendered entity on Views Fixed . Just posting to save others time as that has been committed to 10.1.

smustgrave → credited tar_inet → .

smustgrave → credited sylus → .

smustgrave → credited sjpeters79 → .

smustgrave → credited raman.b → .

Closed 🐛 [regression] getEntity() doesn't always return entity, which results in add comment field not working in Views Closed: duplicate as a duplicate

I can confirm patch on #144 fixed the issue for me on Drupal 9.4.x

Rebased #146 onto an MR against 10.1.x. Setting to NR for core checks as they aren't running locally for me for some reason. Will try to address the last remaining piece of feedback (logging) after.

So in terms of logging, I'm not entirely sure where this is best placed. Putting it in FieldPluginBase::getEntity would make sense but it would have to be a very generic message since we don't have context of what the entity id or type is there.

I think the only place we can really add it is where _entity is set which looks like is in Sql::assignEntitiesToResult. That would mean adding a new dependency on the logger service and that would mean we'd need a deprecation dance for any classes extending that plugin.

Before I go down that route, it would be good to get confirmation from @catch and/or @Berdir that this sounds like the right approach.

FieldPluginBase::getEntity has the following available

• $this->configuration['entity_type'] for the entity-type
• $this->configuration['entity field'] for the field name
• $values->index for the row index
• $this->view->id() for the view machine name

so in theory we could use that information to construct a reasonably informative log message

Back to NW for logging

Logging added. Almost all tests needed updating to mock the logger factory so I've added a trait for that. Also removed the tearDowns which were unsetting the container as that happens in UnitTestCase::setUp anyway.

Almost certain there will be changes needed to the wording but this gets us into that convo :)

tested the MR !3529 , worked fell, all senerio covered, moving to RTBC done

As this is a bug, can we also get a 9.5.x version? Thanks - it doesn't apply cleanly to 9.5.x.

Rolled for 9.5.x

Moving back to RTBC.

Compared patch #164 has all the changes in MR 3529 and they are all there except the change to phpstan-baseline.neon, (because it's not in 9.5)

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. 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.

Moving back to RTBC. Think the bot tried to apply the 9.5.x branch to 10.1.x

New comments on the MR from @mstrelan

Moving to NW for additional comments in MR.

I'll try and address this today

All comments addressed.

The failing tests are related to the Drupal\FunctionalJavascriptTests\Tests\JSWebAssertTest class, specifically to the testJsWebAssert method. The error message suggests that there was a problem with the WebDriver, specifically with a stale element reference. This could be caused by the web page changing before the test could complete, or by an error in the test script itself.

To correct this issue, the code for the JSWebAssertTest class needs to be reviewed and corrected. It is likely that the testJsWebAssert method needs to be updated to account for changes in the web page during testing. Additionally, the WebDriver configuration may need to be updated to ensure that it is properly interacting with the web page

These ChatGPT answers are not helpful.

How is it relevant to this issue?

There still appear to be some open threads in the MR. Know you probably can't resolve them but could you leave a comment on them saying if they've been addressed or not needed.

Weird, I thought I had applied all suggestions in the last push but I seem to have missed 5 of them...all applied now.

Have reviewed the MR and all feedback has been addressed with tests passing. API additions mentioned in the Issue Summary don't need a Change Record. This is good to go.

Updating issue credits

Committed to 10.1.x

Although there is a minor API change here (returning null instead of a Url) at present that code just blows up with a WSOD so I don't think this is disruptive (its a WSOD out of your hands now, it would be something you can do something about with this patch). So on that basis I think this is worth considering for 10.0.x and 9.5.x but I will get a second opinion.

Setting as patch to be ported in the meantime.

This is causing test fails in TMGMT :-/

I have a custom entity label field plugin there: \Drupal\tmgmt\Plugin\views\field\EntityLabel, this displays the label of an entity, and it's on an optional relationship (messages that might or might not have a reference to a job item).

Not only does this now log errors in a situation that is not unexpected, the logging also blindly asumes that there is an entity field in the configuration, which I think isn't necessary. It might be due to very old default views configuration but still, it shouldn't cause php warnings on this:

Warning: Undefined array key "entity field"
Drupal\views\Plugin\views\field\FieldPluginBase->getEntity()() (Line: 438)

I can work around it by checking if I have an ID, but http://grep.xnddx.ru/search?text=$this-%3EgetEntity($values) finds a bunch of similar use cases. I think what this should do is check for optional relationships and not log an error then.

Reverted in light of #182 - let's see if we can make the logging conditional if the relationship is optional

Taking a look

Was a bit tricky to figure out how to reproduce the failure but I was able to by:

1. Checking out this branch on core
2. Cloning tmgmt module
3. Reverting https://git.drupalcode.org/project/tmgmt/-/commit/aab4b531f7cd60bbb5e25e...
4. Running phpunit app/modules/contrib/tmgmt/translators/tmgmt_local/tests/src/Functional/LocalTranslatorTest.php --filter testBasicWorkflow
5. Which failed with Warning: Undefined array key "entity field" Drupal\views\Plugin\views\field\FieldPluginBase->getEntity()() (Line: 438)

I then applied a fix (latest commit on the MR) and ran the tests again and they passed.

It seems like we don't have any tests in core that have an optional relationship with an entity that doesn't load, otherwise that would've triggered the same error? If anyone has pointers on where to put a test like that, that'd be great.

Test pushed. It's pretty gross but does the job. Using a mock logger to save having to install dblog and query for logs.

The change looks good I think, didn't check the test. One part that's not covered yet is handling the case if there's no entity field configuration key, I think that's a valid situation.

Added a guard for #188

Last commit seems to cover #188.

Question is it possible on TMGMT to run tests with this as a patch? Not sure if that's possible in drupalci

Left some questions on the MR

Remarking for committer review

core/modules/views/src/Plugin/views/field/FieldPluginBase.php

The thread for this do we want to open a follow up?

The thread in the MR has some suggestions now

Ok, hopefully this is the one 🤞

Marking this for @acbramley. Checked the threads have been resolved and recommended change for $this->label() ?: $this->realField was added.

This applies and works.

I am placing this note here. It conflicts with 🐛 Cannot use relationship for rendered entity on Views Fixed on Drupal 9.5, which has been added to Drupal 10.1.

Unfortunately, this now needs a reroll, clash in phpstan-baseline.neon

Fine to self RTBC on resolution

Green after merge, back to RTBC

Committed to 10.1.x, moving to 10.0.x for backport

Thanks all 🎉

Here's a reroll for 10.0.x

Reroll looks good.

Committed to 10.0.x, moving to 9.5.x for reroll for that

Seems to have CI errors.

Seems like a good reroll.

At this point, 9.5 is effectively security only.
There are a few commits in HEAD so there may be a bonus bugfix release, but we're not actively backporting any more issues at this point.

Thanks everyone.

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

quietone → closed merge request !3529