Fix the call to a member function getConfigDependencyKey() on null on Widget view and SelectionDisplay view

Re-queuing failed test but the change is simple enough to RTBC after tests pass.

Looks like branch tests are failing https://www.drupal.org/node/1943336/qa, we need to wait for that to be fixed before committing this.

Hello,

Thanks for the patch.

I encountered the same issue.

I have updated entity browser from 8.x-1.0-beta2 to 8.x-1.0-beta4 and when I have rebuilded my website from scratch I got the error and the patch solved it.

I do not change the status to RTBC as I don't know if there is other things to do in the module to solve the issue.

This is a quite important issue. We can not install sites that are using this module.

The branch fail the test, but someone can point which test are failing? issue I mean.

To try to solve ASAP, we can not relay on a patch in our composer.

Code from #4 looks good

@RajabNatshah Thanks for the patch. Applied successfully on 8.x-1.0-beta4

Added parenthesis around the assignment.

Committed. Thanks!

Discussed this with @beridr. He pointed out that the fact that view is not loadable indicated some bigger problem and with this patch we just hide it.

I also noticed that this happens if one is using media module since it does not correctly declare its dependencies: #2849475: Entity browser configuration doesn't correctly declare all its dependencies. Did you get this error using that module?

Check what was done in #2849475: Entity browser configuration doesn't correctly declare all its dependencies (which was just committed). You should do the same thing to fix the problem.

If I understand #2849475: Entity browser configuration doesn't correctly declare all its dependencies correctly, this means that every existing Entity Browser config (exported to a module/distribution's /config/install folder) will need to be updated to include the View dependency. This would effect Lightning, Demo Framework, and Thunder, right? I'm wondering how we can make this easier on people with existing config exports, not sure what the best path forward is if the patch from this issue isn't acceptable. Any ideas?

The approach that this patch uses leaves the doors open for problems in the future and I would rather not use it.

One thing that we could do is to allow view to be non-loadable on the first run and enforce it after that. That would ensure that the install went file even for exported configurations that are missing dependencies with the expectation that all of there were imported. We could use a flag somewhere to track that. I am not sure though if the benefits that this would bring outweigh the additional complexity in the code. It is also worth mentioning that we're already in beta which kind of implies we'll maintain the BC. Not sure about that.

Another approach could be a script that would help people update their config exports. This would obviously still require manual work so it is questionable how much sense this makes.

When we include rc2 in our distribution, it's currently making the installation fail altogether. A fatal error during installation sounds critical :)

<berdir> stefan_r: the proper fix is updating your default configuration to depend on the view then it no longer fails to install
<berdir>  stefan_r: if you use media, then it needs to be fixed there if it wasn't already and if you have custom entity browsers then you need to fix it there. the only "fix" that entity browser could do is give a better exception that tells you which browser/view is broken
<stefan_r> berdir:  ah so just let the entity_browser config that was already there depend on the view... so really the problem is with the distro and not with the module
<berdir> stefan_r: yes, previously the order didn't matter and was not enforced. now drupal imports the entity browser first and then the view, and then it fails on recalculating the dependencies of the browser as the view doesn't exist yet. you should be able to update on an installed site and export the browser configuration there

Thanks Stefan for posting the conversation, that explains a lot.
I had to manually add the views dependency (views.view.yourview) to the entity_browser config (entity_browser.browser.yourbrowser.yml).

So as per the convo with Berdir, why don't we made the module explain which view is causing the problem? It should be a simple addition to src/Plugin/EntityBrowser/Widget/View.php.

Added new patch, as having issue where getConfigDependencyKey method does not exist.

#32 works for me for "drupal/entity_browser": "^2", - Version: 8.x-2.5
And we were able to install in within Open Y distribution + Drupal 9 by using both drush ( 10 ) and web Installer.

The patch in #32 is for version 8.x-1.x.

Attaching an updated patch for 8.x-2.x with the only changes being line numbering for the patch to apply cleanly.

#35 looks good
Let's release it.
Thank you

What's the reason for adding the method_exists($view, 'getConfigDependencyKey') check. Shouldn't this always be defined for every View config entity?

Is the following code a tentative to check that ViewEntity::load($this->configuration['view'])) effectively returns a View object?

   if ($this->configuration['view'] && ($view = ViewEntity::load($this->configuration['view'])) && method_exists($view, 'getConfigDependencyKey') ) {
     $dependencies[$view->getConfigDependencyKey()][] = $view->getConfigDependencyName();
   }

If that is the case, I would rather remove the part checking the value returned from method_exists($view, 'getConfigDependencyKey'). The error reported from the IS is caused by code trying to call getConfigDependencyKey() on NULL. To avoid that, the following code is sufficient.

   if ($this->configuration['view'] && $view = ViewEntity::load($this->configuration['view'])) {
     $dependencies[$view->getConfigDependencyKey()][] = $view->getConfigDependencyName();
   }

Yup if $view is assigned NULL or FALSE the if statement will not pass. Moving to needs work for an update.

I agree. ViewEntity::load() won't return (for example) 1, which would make the if() fail and cause an exception.

Please, review the patch.
I think it would be better to add isset($this->configuration['view']) to avoid generating unnecessary warnings.

Uploaded a new patch.

Since patches are no longer tested, a merge request needs to be provided.

Hi,

I have checked and found that the changes mentioned in #44 are already merged on 8.x-2.x branch. So, no work is needed here.

Thanks!