-
Notifications
You must be signed in to change notification settings - Fork 133
Conversation
Wow there are a whole lot of them! |
I made a bit of progress on this but we are affected by a false positive:
This is thrown for this code, but the variable
The sniff that throws the false positive is I started looking into it but I ran out of my contribution time before making any meaningful progress. |
Just a thought, would the non-shorthand version get us through the hurdle? It may not be ideal but something like:
|
Oh yes it would help OG in this particular instance, but our code is valid, the bug is in the coder module. I'm doing this in my spare time and in my personal philosophy I prefer to spend my time to fix the root of the problem in the Coder module so that this is fixed not only for us but for the whole Drupal ecosystem. Especially since this already affected me before in some project. If this would affect my work project I would be happy with a quick fix, but for me personally fixing this is not super urgent :) I was just working on this because it is now blocking some important PRs like #692 |
I think in this case it would perhaps be a good idea to skip those failing lines that are affected by the false positive, or even disable the faulty sniff entirely. |
Reported the issue in sirbrillig/phpcs-variable-analysis#219. @pfrenssen, this may take some time to get resolved. Personally, I find the sniff Seems that in the meanwhile another issue with phpunit appeared.
|
We can ignore those failing lines that are affected by the false positive, by putting a comment |
Still some sniffs complaining:
|
The array declaration extends to column 81 (the limit is 80). The array content should be split up over multiple lines.
* Use dependency injection for loadMultiple FieldStorageConfig::loadMultiple calls should be avoided in classes, use dependency injection instead. * Remove unused use statement
@pfrenssen or @amitaibu, tests are green. This one's ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on that!
Merged, and pushed to d.o. |
Seems like there's been an update to phpcs;