Up to now we have, I think, used a pretty loose coding standards rules file to avoid fails. Based on the one from Core, if I'm not mistaken.
However, Core is far more lax regarding coding standards than we are, so we should be fine running far more rules. This will also help us much better to avoid accidental regressions in our standards-compliance, which can now more easily happen due to explicitly excluded sniffer rules.
To this end, I'm also re-thinking my position on some of the "uglier" changes which me have to make to comply with some of the rules. This might still be the "lesser evil" than removing the rule altogether, leaving us open to unintended violations elsewhere.
| Comment | File | Size | Author |
|---|---|---|---|
| #7 | 2907334-7--coding_standards.patch | 37.26 KB | drunken monkey |
Comments
Comment #2
drunken monkeyActually, there were just three "problematic" points:
\Drupal\search_api\Query\Query::setDisplayPluginManager()didn't have a type hint for$display_manager. To avoid type-hinting on a class name, I created a new interface,DisplayPluginManagerInterface, for the class and type-hinted on that. Since the plugin manager adds a public method, this does make sense anyways. (Otherwise, I probably wouldn't have done this.) If we're gonna keep that, we just need a change record for this (hence the tag). Also, we should probably commit this immediately after the next release, so other modules have enough time to adjust, in case they need to. (Concerns a bunch of other places, where type hints were added, as well.)SearchApiFieldTrait, where we can't add a type hint because the field handler plugin base class (this being Views) doesn't have them, either. I think with normal inheritance, the sniffer would even be smart enough to realize this and refrain from complaining – however, with a trait, it can't of course know that this is the case.Long story short: In order to keep the sniff, but avoid the complaints, I changed the parameter doc in all cases to
@param array|\ArrayAccess. Nonsense, of course (though technically true, I guess), but the best way I could think of of resolving this.I did the same for the
$contextparameters of our batch callbacks, by the way, but since Drush actually does call them with\ArrayAccessobjects, it actually makes sense there.OK. Reviews, comments and opinions welcome!
Comment #3
drunken monkeyComment #5
borisson_All these changes look solid!
Comment #7
drunken monkeyThanks a lot for reviewing, good to hear that the code style changes look OK to you.
Obviously, though, they did break a few things here and there. The attached revision should fix that.
At least, though, this also gave me time to create a change record.
Comment #8
borisson_Comment #10
drunken monkeyGreat, thanks again for the review!
Committed.