Problem/Motivation

When a null value is passed into EntityStorageBase::load() it gets passed into loadMultiple as [0 => NULL], which triggers this warning that doesn't provide much help to developers:

Warning : array_flip(): Can only flip STRING and INTEGER values!

Proposed resolution

Add an assertion to the load method that the ID is not null.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom created an issue. See original summary.

jhedstrom’s picture

Title: Provide a better error when a NULL is passed to EntityStorageBase::loadMultiple » Provide a better error when a NULL is passed to EntityStorageBase::load()
Issue summary: View changes
jhedstrom’s picture

Status: Active » Needs review
FileSize
549 bytes

This will probably need a test, but pushing up here to see if other tests fail.

Gogowitsch’s picture

First of all: I am on board with the catching this typical mistake. However, I don't think assert will throw it into developers' faces, because by default, the assert system is not enabled in my environments. That might be the Ubuntu default, because I have not touched it. In XAMPP on Windows the assert was enabled. In summary, I believe we have to throw an exception.

jhedstrom’s picture

Given how common this error is, in contrib and certainly in custom code, I'm not sure an exception would be the way to go as that would be a breaking change rather than a better error message. Currently a PHP warning is thrown, which would be disabled on most production sites.

Having assertions on is in the recommended developer settings.

jhedstrom’s picture

The last submitted patch, 6: 3035980-06-TEST-ONLY.patch, failed testing. View results

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Gogowitsch’s picture

Status: Needs review » Reviewed & tested by the community

+1 from me. Thanks for pointing me to enable assert. I updated all our dev and continuous integration environments.

Gogowitsch’s picture

This is the unchanged patch from #6, all credit belongs to jhedstrom.

I am uploading it to force the test runner to apply it to the 8.8.x branch, so it can be merged by the core maintainers with more confidence.

  • larowlan committed 4dfdaae on 8.8.x
    Issue #3035980 by jhedstrom: Provide a better error when a NULL is...

  • larowlan committed 99c6cfc on 8.7.x
    Issue #3035980 by jhedstrom: Provide a better error when a NULL is...
larowlan’s picture

Version: 8.8.x-dev » 8.7.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 4dfdaae and pushed to 8.8.x. Thanks!
c/p as 99c6cfcca8 and pushed to 8.7.x

Status: Fixed » Closed (fixed)

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