Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.When reviewing the overrides on a feature, got repeated instances of this error. custom_formatters_features_pipe_field_alter() attempts to iterate over $field['display'] without first ensuring that it is an array.
| Comment | File | Size | Author |
|---|---|---|---|
| #5 | custom_formatters-1815948.patch | 1.33 KB | Ken Ficara |
| #1 | features-1815948.patch | 988 bytes | Ken Ficara |
Comments
Comment #1
Ken Ficara CreditAttribution: Ken Ficara commentedPatch attached, checking that $field['display'] is set and is an array.
Comment #2
hefox CreditAttribution: hefox commentedThanks for the patch. Few problems
* Filed against wrong project -- customer formatters provides intergrates with features, so the code is in custom formatters itself
* Patch rolled from wrong location (sites/all/modules instead of drupal root)
* Coding standards -- wrong spacing and fairly certian it's missing a closing tag
!empty() is likely all that's needed instead of isset and !is_array -- if it's not empty it better be an array else that's a bug outside of custom formatters
Comment #3
Deciphered CreditAttribution: Deciphered commentedHappy to fix the issue if you can provide some reproduction steps so I can confirm the issue exists and that the fix is the correct fix.
Comment #4
Ken Ficara CreditAttribution: Ken Ficara commentedAnd I would be happy to reroll the patch and fix the formatting, but it's easy to reproduce. Pass a field with a nonexistent display element, or one that is not an array, to the function. Boom, PHP error that stops the site in its tracks. Yes, you should be able to depend on it being there and being an array, but apparently you can't. OK, no fault no foul. Add an extra error check you don't really need and move on.
It doesn't matter whose fault it is; it's not meaningful to anyone but us developers that the fault was not in custom formatters, but in the data that were passed. The end user just knows that suddenly s/he cannot revert the feature anymore, and probably won't be able to figure out the problem if s/he cannot look at the code. How else could an end-user diagnose this, other than by removing components one-by-one from the feature until it stops blowing up? Even as a programmer, it would take a lot of extra work dumping variables to figure out which field was passing the bad data.
We all have to make sure we handle things even when they're not correct. it doesn't matter whose fault it is. Remember Postel's law: "Be conservative in what you send, liberal in what you accept." This check shouldn't be necessary, but it is. Add a belt to the suspenders, and let's hope that D8 will improve things. It's hard to pick a favorite XKCD, but I do love Little Bobby Tables. I just hope he changed his last name to 'DROP TABLE Employees; when he got his first job....
I'll fix my errors and resubmit the patch. Thanks for considering it.
Comment #5
Ken Ficara CreditAttribution: Ken Ficara commentedOK, here is the re-rolled patch. And I should have been clearer in my last reply that I was answering both you and hefox -- I know this just showed up in your queue after hefox moved it over from where I originally mis-posted it. I hope this one is good. Thanks again.
Comment #6
Deciphered CreditAttribution: Deciphered commentedTurns out that this issue was fixed a long long time ago, yes only in dev, and I probably should release a new stable at some stage in the future, but nevertheless it is a resolve issed.
Comment #7
Deciphered CreditAttribution: Deciphered commentedActually, not only is it fixed, it is fixed in 2.2, are you using an older version of Custom Formatters?
Comment #8
Ken Ficara CreditAttribution: Ken Ficara commentedNope, using 2.2 -- just downloaded a fresh version to be sure, and custom_formatters_features_pipe_field_alter() still looks as it was pre-patch. Maybe there were changes elsewhere that should have caught the problem first? If so, something is making its way around those checks. It looks like 2.x-dev is still using that variable without checking it first. Perhaps I'm overly conservative, but in a weakly typed language like PHP, I prefer a belt-and-suspenders approach to handling data.
Comment #9
Deciphered CreditAttribution: Deciphered commentedMy bad, I read the patch wrong while I was concentrating on something else. I'll take another look at this shortly and try to confirm the issue.
Comment #10
Deciphered CreditAttribution: Deciphered commentedI'm going to mark this issue as fixed for the following two reasons:
- Features 7.x-2.x will no longer trigger that function due to the changes in Fields (fix for that about to be committed).
- I'm thinking that display not being an array is not something that should happen.
However, if this issue does ever come up, or if anyone knows why display could be anything other than a populated array I will be happy to revisit.
Comment #12
COBadger CreditAttribution: COBadger as a volunteer commentedReopening with apologies if I should have created a new issue, but we have features-7.x-2.7 and custom_formatters-7.x-2.4, and one of our feature modules is throwing this error.
Patch #5 solves this problem for us.
Setting status to RTBC.
Comment #13
Deciphered CreditAttribution: Deciphered commentedThis fix is in the dev release. Should there be a new stable? Absolutely. Is this issue still open? No.
I will be releasing a new Custom Formatters stable just prior to beginning my D8 port, but my priority has been on other modules of later.