Closed eileenmcnaughton closed 4 years ago
@colemanw also ping from here - I think we can enable this with the above merged
I would be a +1 on this it will improve the code and there weren't heaps of places i found missing but we can just do it as we hit each one @totten
I just ran find Civi CRM api bin extern -name '*.php' | grep -v /examples/| time xargs -P3 -L20 phpcs-civi
with this PR applied locally and the results were https://gist.github.com/seamuslee001/69fb47e7f11140f625f21d76a9e59d54 which i believe shows this is safe to merge in @totten
To spot-check, I ran this on the Civi
folder and have mixed feelings about the warnings.
@var
elements. Some examples like @var DAO Object
should definitely change. It's good for the linter to highlight that.@var
. This means that canonically valid examples from PHPDoc (https://docs.phpdoc.org/latest/references/phpdoc/tags/var.html) become arbitrarily invalid in Drupal. (And don't get me started on @param
which is similarly whiny about new-lines and which diverges from PHPDoc, Javadoc, JSDoc, YARD, ad nauseum...) Regardless, this does seem to be a valid interpretation of Drupal's @var
convention.My personal reaction to Drupal.Commenting.VariableComment.IncorrectVarType
is kind of "meh", but it is a proper+consistent thing to do.
A silver-lining: if we look forward to some day using PHP 7.4 typed properties... the @var
annotation will eventually be unnecessary, and I think it'll be a bit cleaner to drop @var
from Drupal's style (wherein substantive description has already been moved up to the top).
@totten my hope is we work towards the full set being enabled but I thought if we enabled requiring types before enforcing correct types someone would just add a lot of incorrect types to meet that requirement so it makes sense to get this enabled first
@eileenmcnaughton . @totten and I chatted in the infrastructure channel in mattermost and i believe we have come to an agreement this would be merged at the start of the next RC as that will mean that the current stable, RC and Master would all then have the fixes we have just put in recently.
@totten @seamuslee001 I ran phpcs with this change & while it surfaced a number - see https://github.com/civicrm/civicrm-core/pull/14296/files and https://github.com/civicrm/civicrm-core/pull/14294 - I feel like the incidence is low enough that it would not impose undue burden to remove this one & raise our bar a little. I think there are still a couple left in the DAOs with those 2 merged (timestamp needs to be string) but I think it's pretty trivial to resolve