EntityNG uses Language however doesn't declare thus causing certain circumstances to error.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nick_schuch’s picture

Status: Needs work » Needs review
FileSize
458 bytes
Berdir’s picture

Issue tags: +Needs tests

makes sense but probably needs a test to verify. When does this happen?

We could also say that we don't care much as it will get merged back into Entity and postpone this issue...

larowlan’s picture

Looking at the code I think an entity create without a langcode then attempt to access the language would throw an error, but it would be a fatal. Not sure how we'd test that.

aspilicious’s picture

you can test fatals with a try catch.

larowlan’s picture

Is that new? I thought fatals couldn't be caught.
Happy to be wrong on that.

Berdir’s picture

No, they can't.

But a fatal error in a test is a test fail, so that's fine.

To get the fix in, we need test coverage. However, I'm not sure if we should add more NG specific tests as we want to merge all *NG files back into * anyway and I assume that we're going to stumble over this sooner or later when doing the real conversions.

larowlan’s picture

So should we just add the fix at #1 for now to save someone getting a wtf during conversions and add a postponed follow up for tests if it is still applicable after the *NG merge into *?

Berdir’s picture

Status: Needs review » Closed (duplicate)

Yeah. I didn't expect that it would happen that fast, but #293318: Convert Aggregator feeds into entities did run into that problem and I fixed it over there. That means we also have test coverage for it, so closing this issue as a duplicate.