Problem/Motivation

When selecting "Views: Filter by an entity reference view" as a reference method on a entity_reference field's settings a fatal error occurs.

Fatal error: Using $this when not in object context in /var/www/html/d8.local/drupal/core/modules/views/src/Plugin/EntityReferenceSelection/ViewsSelection.php on line 252

Proposed resolution

$form_state->setError($element, $this->t('The views entity selection mode requires a view.'));

settingsFormValidate is called outside of an object context so we should use t() here instead of $this->t().

Remaining tasks

Write a patch
Write test

User interface changes

None

API changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because fatal error occurs
Issue priority Major because php error is thrown when doing a simple action through the interface.
Prioritized changes The main goal of this issue is fixing a bug.
CommentFileSizeAuthor
#36 2430813-36-complete.patch5.98 KBgeertvd
#31 2430813-31-complete.patch5.8 KBgeertvd
#31 interdiff.txt1.71 KBgeertvd
#22 interdiff.txt3.37 KBgeertvd
#22 2430813-22-complete.patch5.98 KBgeertvd
#19 2430813-19-complete.patch3.7 KBgeertvd
#19 2430813-19-test.patch1.29 KBgeertvd
#14 2430813-14-complete.patch2.42 KBgeertvd
#14 2430813-14-test.patch1.3 KBgeertvd
#8 2430813-8-complete.patch2.34 KBgeertvd
#8 2430813-8-test.patch1.22 KBgeertvd
#2 2430813-2.patch779 bytesgeertvd
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

geertvd’s picture

geertvd CreditAttribution: geertvd commented
Issue summary: View changes
geertvd’s picture

geertvd CreditAttribution: geertvd commented
Status: Active » Needs review
FileSize
2430813-2.patch779 bytes
geertvd’s picture

geertvd CreditAttribution: geertvd commented
Issue summary: View changes
geertvd’s picture

geertvd CreditAttribution: geertvd commented

This should probably be a static method also.

geertvd’s picture

geertvd CreditAttribution: geertvd commented
Status: Needs review » Needs work
geertvd’s picture

geertvd CreditAttribution: geertvd commented
Issue tags: +Needs tests
geertvd’s picture

geertvd CreditAttribution: geertvd commented
Issue summary: View changes
geertvd’s picture

geertvd CreditAttribution: geertvd commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
2430813-8-test.patch1.22 KB
2430813-8-complete.patch2.34 KB

Added test and made settingsFormValidate a static method.

geertvd’s picture

geertvd CreditAttribution: geertvd commented
Issue summary: View changes

The last submitted patch, 8: 2430813-8-test.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 8: 2430813-8-complete.patch, failed testing.

Status: Needs work » Needs review

geertvd queued 8: 2430813-8-complete.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 8: 2430813-8-complete.patch, failed testing.

geertvd’s picture

geertvd CreditAttribution: geertvd commented
FileSize
2430813-14-test.patch1.3 KB
2430813-14-complete.patch2.42 KB
geertvd’s picture

geertvd CreditAttribution: geertvd commented
Status: Needs work » Needs review

The last submitted patch, 14: 2430813-14-test.patch, failed testing.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Issue tags: -Needs tests

The patch and test looks great, just one tiny nitpick:

+++ b/core/modules/entity_reference/src/Tests/EntityReferenceAdminTest.php
@@ -28,7 +28,7 @@ class EntityReferenceAdminTest extends WebTestBase {
+  public static $modules = array('node', 'field_ui', 'entity_reference', 'path', 'taxonomy', 'block', 'views', 'views_ui');

We don't need to enable the views_ui module :)

geertvd’s picture

geertvd CreditAttribution: geertvd commented

Actually, this is an issue I bumped into while writing the test, at the moment views_ui needs to be enabled because we are printing Url::fromRoute('views_ui.add') here 

$form['view']['no_view_help'] = array(
        '#markup' => '<p>' . $this->t('No eligible views were found. <a href="@create">Create a view</a> with an <em>Entity Reference</em> display, or add such a display to an <a href="@existing">existing view</a>.', array(
          '@create' => Url::fromRoute('views_ui.add'),
          '@existing' => Url::fromRoute('entity.view.collection'),
        )) . '</p>',
      );

How do you propose we should fix that? It's a bit silly that views_ui needs to be enabled for this.

geertvd’s picture

geertvd CreditAttribution: geertvd commented
FileSize
2430813-19-test.patch1.29 KB
2430813-19-complete.patch3.7 KB 
+++ b/core/modules/views/src/Plugin/EntityReferenceSelection/ViewsSelection.php
@@ -120,12 +120,18 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
+      $account = \Drupal::currentUser();
+      if ($account->hasPermission('administer views') && \Drupal::moduleHandler()->moduleExists('views_ui')) {
+        $form['view']['no_view_help'] = array(
+          '#markup' => '<p>' . $this->t('No eligible views were found. <a href="@create">Create a view</a> with an <em>Entity Reference</em> display, or add such a display to an <a href="@existing">existing view</a>.', array(
+            '@create' => Url::fromRoute('views_ui.add'),
+            '@existing' => Url::fromRoute('entity.view.collection'),
+          )) . '</p>',
+        );
+      }
+      else {
+        $form['view']['no_view_help']['#markup'] = '<p>' . $this->t('No eligible views were found.') . '</p>';
+      }

Forgot to take an interdiff but this is what I added

amateescu’s picture

amateescu CreditAttribution: amateescu commented 
+++ b/core/modules/views/src/Plugin/EntityReferenceSelection/ViewsSelection.php
@@ -120,12 +120,18 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
+      $account = \Drupal::currentUser();
+      if ($account->hasPermission('administer views') && \Drupal::moduleHandler()->moduleExists('views_ui')) {

If we're going down this path, the current_user and module_handler services can be injected in the plugin.

The last submitted patch, 19: 2430813-19-test.patch, failed testing.

geertvd’s picture

geertvd CreditAttribution: geertvd commented
FileSize
2430813-22-complete.patch5.98 KB
interdiff.txt3.37 KB

Something like this then

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Title: Selecting a reference method on an entity_reference field causes a fatal error » Selecting the 'views' selection handler on an entity_reference field causes a fatal error
Status: Needs review » Reviewed & tested by the community

Perfect!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs review

Why does #element_validate have to be static? I.e can't we do

$form['view']['#element_validate'] = array(array($this, 'settingsFormValidate'));
amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Needs review » Reviewed & tested by the community

By declaring element validate (also process, etc.) handlers as a static methods we skip a lot of form object serialization. Field API does this consistently all over the place :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

Huh http://3v4l.org/S0onn?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs review

Basically I don't get the argument in #25 - using $this will not save any form serialisation and if the form is serialised the object detection in serialisation might well end up being more memory efficient that storing the classname.

amateescu’s picture

amateescu CreditAttribution: amateescu commented

When you start adding objects (e.g. entities, services) to $d, all of them will need to go through the serialization process, while the simple array(array('\some\class\name', 'someMethod')) will not.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

Yeah but this is $this

amateescu’s picture

amateescu CreditAttribution: amateescu commented

I don't understand #29.

geertvd’s picture

geertvd CreditAttribution: geertvd commented
FileSize
interdiff.txt1.71 KB
2430813-31-complete.patch5.8 KB

Just changed

$form['view']['#element_validate'] = array(array(get_called_class(), 'settingsFormValidate'));

to 

$form['view']['#element_validate'] = array(array($this, 'settingsFormValidate'));

I guess this makes sense, since settingsFormValidate is only called from within ViewsSelection I don't see why it should be static.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

re #29 - there is no saving when doing get_called_class($this) because $this is what is being serialised.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

It's not the $this that is serialized. It's $form, containing references to $this. And if we make that static, then we can avoid that being serialized. As @amateescu said, field api widgets use that pattern a lot.

Being able to do this is one of the main reasons I introduced support for '::methodName' , but that only works in the actual form objects.

See http://3v4l.org/ALTTj

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented

I agree that we should serialize as less as possible to avoid deep-object-references.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs review » Needs work

Ok get_called_class it is - not going to fight - but it makes code less testable and dependencies less obvious.

I agree that we should serialize as less as possible to avoid deep-object-references.

We have object serialization going on all over the place.

geertvd’s picture

geertvd CreditAttribution: geertvd commented
Status: Needs work » Needs review
FileSize
2430813-36-complete.patch5.98 KB

Same patch as in #22

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Needs review » Reviewed & tested by the community
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Assigned: Unassigned » alexpott

Looks like Alex has been all over this one.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 9dfa950 and pushed to 8.0.x. Thanks!

  • alexpott committed 9dfa950 on 8.0.x 
    Issue #2430813 by geertvd: Selecting the 'views' selection handler on an...

Status: Fixed » Closed (fixed)

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