EntityMalformedException: Missing bundle property error when entity reference selection mode is set to Views: Filter by an entity reference view

These are related I believe and depending on how the maintainers decide to deal with it, they may deal with the problem here or there.
#2151631: Improve performance of getReferencableEntities(). I won't close this as a duplicate yet, but probably should close it if the other gets committed as a revert or partial revert.
Generally new issue per problem, so maybe committing it over here is better?

Try this patch

alternatively, try this patch

patch 4 should work.
This one might also resolve the issue.

@joseph.olstad I'm not a fan of the try catch approach because you're passing in the wrong data type as $entity on purpose but it also suffers from the same problem as mentioned to @franceslui here
#2151631-48: Improve performance of getReferencableEntities()

@franceslui, you probably need to add a empty check after the entity_load in case it was not loaded due to access control or something for #37

And #6 is a bit better although
list($id,, $bundle) = entity_extract_ids($target_type, $entity); will suffer the same fate if the resulting entity_load does an access check and notices you aren't allowed to load that entity.

Minor nitpick for #6 as I've seen you do this in other patches I might as well mention it. empty() does an implicit isset(), so you don't need both.

if (!isset($entity->{$info['entity keys']['bundle']}) || $entity->{$info['entity keys']['bundle']} === '') {
becomes
if (empty($entity->{$info['entity keys']['bundle']})) {

Though I'm still in favor of @franceslui's patch (with a check for a returned entity after access checks have run) #2151631-37: Improve performance of getReferencableEntities()

@joelpittet, Did the patch #6 resolve the issue for you? I used the verbatim exact condition that core does in entity_extract

that line is straight from core here:

https://api.drupal.org/api/drupal/includes%21common.inc/function/entity_...

it should prevent the exception.

@joelpittet, re: patch #6.
I believe core does this due to the faster performance of isset.
I merely copied what core did verbatim.
(from here: https://api.drupal.org/api/drupal/includes%21common.inc/function/entity_...)
In fact, you were behind some related micro optimizations
#2770065: array_key_exists() micro-optimization

the reasoning behind the micro optimizations is that isset is very fast.

This might not be the case in PHP 8.1+ but it was back when we were doing performance profiling on #2770065: array_key_exists() micro-optimization

ok the empty check, I'll roll a new patch

@joelpittet, I'm hopeful that this patch will do the job for you.

with that said, check the results of try catch, it's perhaps the fastest performing approach.

https://stackoverflow.com/a/445094

For now I'm going with the approach of patch 6 and patch 12, but according to stackoverflow the performance of try/catch is extremely high.

If you want the empty condition instead of the isset I can replace that also , with that said, I'm going straight from the Drupal core API approach for the entity_extract_ids() for consistency / performance I think it would be good to follow what core is doing in this case.

oops , scratch 12, going with 14

Yes, I have much experience with micro-optimizations 😅, array_key_exists() is slow, isset($array[$key]) || array_key_exists($key, $array) is faster because PHP shortcuts on a positive isset() check, but you need both because they do different things.

Nice work on referencing core, but I stand by what I said in #8, and with an implicit isset(), empty() is really fast too.

This issue is really wanting of a regression test... it's probably the best way to make sure any changes we make are going to survive logical mistakes in the code.

#12 the reset() should be on $loaded_entities EDIT: looks like you spotted that too, lol. But let me take a moment to write a failing test then you can throw patches at it, sound good?

Ok @joelpittet, the performance cost of empty() is multitudes times less than entity_load() so I'm totally sure that the performance difference is minute, with that said it is in a foreach loop.

Here's the new patch as requested.

omg, what was that, ok undo

removing noise from .orig file

@joseph.olstad ok here is the tests-only patch. Add your patches that you'd like to test against it. It tests all the things I want (except translation, which might also be affected...).

I realized while writing the test that while taxonomy_term_access does matter, it also gets called by the View execution before, so it's getting a double duty with the entity_load. The test covers the taxonomy_term_access just in case.

This patch is expected to fail (showing the problem with #2151631: Improve performance of getReferencableEntities())

@joelpittet, awesome work, ok testing.

new patch, fix PHP 8.2 deprecation notice

oops, try again

new patch

testing

@joelpittet, neither your colleagues approach, nor my approach resolve this issue, have you tested his approach and my approach?

I will attempt to revert the previous commit in the form of a patch and test against your test code to prove whether or not your test is working.

@joseph.olstad I tested the partial revert. Probably should have posted that too (it was early in my defence). I'll let you try that one. You can run them all locally if you have a setup, though the testbot is always here for us.

Let me know if you want me to post that too if it's giving you trouble still when you do it, maybe I made a mistake somewhere.

Ok, I've switched to testing in my dev environment, it's much faster, I hope to have a working patch soon, thanks for your followup, I'll post a fix as soon as I find it.

Ok this patch should fix the bug. Passes all tests locally.

Try this instead.

That's very strange, locally patch 33 is passing 100% on PHP 8.2.1

passes on my local, see screenshot:

 Drupal version                  :  7.95                               
 Site URI                        :  http://default                     
 Database driver                 :  mysql                              
 Database hostname               :  localhost 

@joelpittet, any idea why patch 33 would pass 100% locally for me on a similar version of php (PHP 8.2.1 vs PHP 8.2.x on d.o?) but fail on d.o?

@joelpittet, the only reason why I could imagine such a difference is Drupal.org test framework uses PHP 8.2.0
my test environment uses PHP 8.2.1

Could there really be such a difference between 8.2.0 and 8.2.1? perhaps!

ok, circumventing the test this time.
Makes no sense, passes 100% locally for me without doing this isset check in the tests.

I don't like having to do this, but the patch passes locally with PHP 8.2.1 no issues, drupal.org test framework appears to be using .0 minor releases first releases of PHP with lots of bugs.

The latest looks to pass, I’ll investigate. My computer is getting repaired this morning. You can discuss on slack if your there.

@joelpittet, ok I'll try to set up slack, I'm on drupalchat.me

Ok just undid the test fixes and cleaned up the assertions to be a bit more helpful. I'll dig into the CI if it fails again.

Ok maybe the reference was going sideways. Trying to simplify the pass by reference issues that may be happening here.

Like you mentioned I can't reproduce the fails on my machine, even running like testbot does:
php scripts/run-tests.sh --color --concurrency "32" --directory sites/all/modules/contrib/entityreference

<error message="exception: [Warning] Line 1871 of includes/bootstrap.inc: Trying to access array offset on value of type int exception: [Warning] Line 1871 of includes/bootstrap.inc: Trying to access array offset on value of type int exception: [Warning] Line 1871 of includes/bootstrap.inc: Trying to access array offset on value of type int exception: [Warning] Line 1871 of includes/bootstrap.inc: Trying to access array offset on value of type int exception: [Warning] Line 1871 of includes/bootstrap.inc: Trying to access array offset on value of type int exception: [Warning] Line 1871 of includes/bootstrap.inc: Trying to access array offset on value of type int " type="exception"/>
This is the only thing in the CI that looked suspicious and that was my bad in the test trying to add the @count

@joelpittet, thanks for looking into this, these kinds of mysteries are fascinating.

I was thinking, to reproduce the drupal.org test environment, we could maybe try installing Drupal vanilla on a docker image with PHP 8.2.0 instead of 8.2.1+?

@joelpittet

for these fixes:
Creation of dynamic property EntityReference_SelectionHandler_Broken::$field is deprecated

this is the fix:

diff --git a/plugins/selection/abstract.inc b/plugins/selection/abstract.inc
index a4805b1..fcbf11e 100644
--- a/plugins/selection/abstract.inc
+++ b/plugins/selection/abstract.inc
@@ -74,6 +74,8 @@ interface EntityReference_SelectionHandler {
  * A null implementation of EntityReference_SelectionHandler.
  */
 class EntityReference_SelectionHandler_Broken implements EntityReference_SelectionHandler {
+  public $field;
+  public $instance;
   public static function getInstance($field, $instance = NULL, $entity_type = NULL, $entity = NULL) {
     return new EntityReference_SelectionHandler_Broken($field, $instance, $entity_type, $entity);
   }

Thanks @joseph.olstad this puts those fixes you made earlier for the abstract.inc but it sounds like it's getting a broken handler, which makes me think the class loader is the problem.

I might come to the same conclusion you have in #51 but it's hard for me to believe that PHP 8.2.0 is the culprit. Do you have one available? I'm just using DDEV with PHP 8.2.3 (earlier I was using PHP 8.1)

I'm looking at a PHP 8.2.0 setup now

jakubboucek/lamp-devstack-php:8.2.0-debug

https://hub.docker.com/r/jakubboucek/lamp-devstack-php

I refactored the fix to use empty(), that's not going to help but the changes in interdiff-58.txt

EDIT: I added a handler check and did some syntax error clean-up... just in case... in the test

Views is not loaded before entityreference and the handler is not available. I see another test that has a workaround. Going to try that.

ok so got the docker environment running but I have to change the port it's on so that the tests will run, I'll try changing 8080 to 80.

@joelpittet, I tested the PHP 8.2.0 theory, it passes ALL tests with patch #33 above

When using a Docker jakubboucek/lamp-devstack-php:8.2.0-debug (PITA to set up btw, but I got it working) DEBIAN BASED.
all tests pass for 8.2.0
When using my normal development server, Ubuntu 18.04 LTS with PHP 8.2.1
all tests pass for 8.2.1

However patch 33 does not pass all tests with Drupal.org infrastructure, and I have no idea why.

Pretty sure that one of my servers has apcu enabled, the above docker might not, but it passes all tests with Patch 33.

Ok proof that all tests pass with patch 33 on the php 8.2.0 docker container

check this:

@joelpittet, I lined up patch 41 that like patch 33, should resolve the issue, unless you found something else. Patch 41 basically ignores the noise from drupal.orgs testbot, makes no sense why this would always pass locally but not on d.o, I even tried php 8.2.0 same same.

I even went as far as switching from fast-cgi to fpm on 8.2.1, the only thing I didn't do was test both fast-cgi and fpm on 8.2.0, however I did test 8.2.0 fast cgi, and it passes with patch 33.

So, do we take #41 as a win, and defer the rest for another ticket?

I'd like to tag 1.8 with this fix, all I need is for you to test patch 41 against 1.7 in your environment where this WAS a problem, and let me know if it resolves the issue.

#60 is the fix I’d like to see in, I’ll keep trying to uncover the secrets of the testbot only failures.

I updated the ci testbot issue title, haven't gotten any traction on it yet, was hoping maybe someone else would notice something or report something similar from another project. There is something about the drupal.org test setup that is different, but what? What could possibly be causing this, we've tested PHP 8.2.0 exactly same minor version, I even tested fast-cgi vs fpm, same success on my server, why is drupal.org test ci failing for this particular test?

#3349436: entityreference patch passes all tests everywhere except on Drupal.org test framework

Have you tried running drupalci locally? It might give you extra clues.

@DamienMcKenna is there documentation on how to set that up? I haven't tried that yet.
Edit: I found it https://www.drupal.org/node/2487065 by following Joseph's link above to the project page of the drupalci test runner

This patch has the instance passed in to the handler (hailmary... again)

https://www.drupal.org/drupalorg/docs/drupal-ci/running-drupalci-locally

Thanks @DamienMcKenna I failed out of the gate on that because of Apple Silicon doesn't like virtualbox
Error: Cask virtualbox depends on hardware architecture being one of [{:type=>:intel, :bits=>64}], but you are running {:type=>:arm, :bits=>64}.

This change is trying to copy our feeds integration on how to enable modules.

#72 is back to EntityReference_SelectionHandler_Broken, which I assume means that Views wasn't installed before entityreference and this failed.
if (module_exists('views')) { in plugins/selection/views.inc

Tweaking this to get views to install first.

Hmm, maybe try this after the setup
drupal_flush_all_caches()

@joseph.olstad $this->resetAll() line does do that, though yeah I was thinking similar.

grasping at straws but maybe some ci issue with depencency detection
hmmm:
parent::setUp(array('views'));

views requires ctools, maybe explicitly include it in the setUp ?

to
parent::setUp(array('ctools', 'views'));

or
parent::setUp(array('ctools', 'views','entityreference', 'entityreference_views_test'));

@DamienMcKenna great suggestion, I'll work on that soon hopefully make progress on running the d.o drupalci dockers, meanwhile, maybe @joelpittet figures it out before I do with a change to the test or module code?

More assertions and tweaking the setUp() module load.

is the views_ui module needed to test the view access?

Taking from how setUp is done in EntityReferenceFormTestCase which uses the same view . Not sure why the access failure on the view built in the same method named createNodeEntityReferenceView.

We don't have the views_ui on EntityReferenceFormTestCase @joseph.olstad

Ha I collided with @joseph.olstad on the last comment and it says I uploaded his file 3348756-interdiff_06_to_12.txt We are hacking d.o now...

I work with @joelpittet.

Because @joelpittet was not able to install DrupalCI Testbot on his machine, I installed it on my machine and could reproduce the errors in it locally.

Sorting that label issue out (hopefully)

I was able to reproduce the error above, the handler is broken... this seems like I have something I can debug so that's helpful.

Views plugin cache is hardcore and the way we are clearing it for the other test is not working for this test.
Seeing how it's done in views tests and not trying to mess with the module load order:
views_fetch_plugin_data(NULL, NULL, TRUE); clears it.

More cache clearing?

Removed that cache clearing and specifically clearing the ctools one.

If we are getting a broken selection handler EntityReference_SelectionHandler_Broken, we know we have entityreference enabled.

Can't get the ctools plugin. Clearing more related caches.

Copy the way FeedsMapperFieldTestCase does the module install.

Tricky to fix the views selection plugin after it's installed, so loading views before should help. I took the cache clearing out for now.

So that module ordering is totally the reason for the plugin not loading. The views selection plugin is wrapped in

if (module_exists('views')) {

And if entityreference is enabled first then that is false and that plugin is cached (apparently pretty hard) as being empty.

Ok taking full control over the module dependencies and ordering.

@joelpittet! wow nice work!

That's impressive! Great work!

Very pleased to get this fix in, major performance improvement (plus memory heap exception fix) can be maintained and we now have added test coverage for the regression fix.

The only thing I'd like to know is WHY drupal.org ci docker image behaves differently than all the environments I tested?

Ok, we're good, aside from the pgsql mystery, (it was passing for 1.7, and the fails are for feeds not what we changed, makes no sense other than possibly some new configurations made to the drupal ci testbot related to pgsql 13.5)

Thanks very much to the entire team at UBC for this important fix and tricky ci mystery fix and for the additional support we got from everyone else.
Release 1.8 is ready here:
https://www.drupal.org/project/entityreference/releases/7.x-1.8

Lots of failures brings deeper understanding… I hope

please try the latest release, there were two reported issues with 1.7/1.8 , patched and released 1.9
rushed this a bit

https://www.drupal.org/project/entityreference/releases/7.x-1.9