Coming from #999530-14: Upgrade Media Gallery for Media's new File Entity (EntityFieldQueryException: Field storage engine not found)

When calling fieldCondition($field_name) or fieldOrder($field_name) where field_info_field($field) returns NULL (field name doesn't exist), we fail in strange, non tale-telling ways - possibly with a "Field storage engine not found" exception down the road if the field was the only one in the query, or with (I guess, untested) a SQL error of there were other fields.

CommentFileSizeAuthor
#1 efq_invalid_field-1038030-1.patch1.29 KByched

Comments

yched’s picture

yched commented
Status: Active » Needs review
StatusFileSize
new efq_invalid_field-1038030-1.patch1.29 KB

Attached patch detects the error earlier on.

bojanz’s picture

bojanz commented

#1: efq_invalid_field-1038030-1.patch queued for re-testing.

bojanz’s picture

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

Patch still applies (although with -p0), tests still pass, approach makes sense.
I was actually about to write this patch myself (wrote EntityFieldQuery support for Per-Bundle-Storage and was thinking how unfriendly the unknown field name business is), and stumbled into the issue by accident.

yched’s picture

yched commented
Version: 7.x-dev » 8.x-dev

Thks bojanz. Bumping to 8.x first.

catch’s picture

catch
he/him
Primary language English
commented

Looks good to me too.

jackbravo’s picture

jackbravo commented

This doesn't seem to add any new functionality, and instead fix something that's very confusing in Drupal 7. Is it too dangerous to port this to D7?

bojanz’s picture

bojanz commented

@jackbravo
It will land in 7.x after it lands in 8.x
Standard procedure.

yched’s picture

yched commented
Issue tags: +Needs backport to D7

Missing the appropriate tag, though

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Definitely a step forward. As a follow-up issue for D8, we should come up with a filtered/optimized backtrace debug output for these kind of exceptions, since although this patch adds the info about what field name is unknown, you still have no effin' clue from where in your code base this field was tried to be queried in the first place.

yched’s picture

yched commented

@sun re: lack of backtrace info :
true - on that note, see #1067750: Let Field API fail in a tale-telling way on invalid $entity, which desperately needs a bump :-)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Issue tags: +String freeze

Hm. This is going to break string freeze in D7, but I think the consequence is worth it.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Issue tags: -String freeze, -Needs backport to D7

#1: efq_invalid_field-1038030-1.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +String freeze, +Needs backport to D7

The last submitted patch, efq_invalid_field-1038030-1.patch, failed testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs work » Fixed

Sorry, I see what happened. This is a patch from CVS, so needs patch -p0.

Committed to 8.x and 7.x.

Status: Fixed » Closed (fixed)
Issue tags: -String freeze, -Needs backport to D7

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

Issue tags: +String freeze, +Needs backport to D7

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.