Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix php comments #22515

Merged
merged 1 commit into from
Jan 16, 2022
Merged

Fix php comments #22515

merged 1 commit into from
Jan 16, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Jan 15, 2022

This commit contains 14 files worth of changes from the following steps

  1. install https://github.com/civicrm/coder.git
  2. comment out <rule ref=Drupal.Commenting.FunctionComment.IncorrectParamVarName><severity>0</severity></rule>
  3. run phpcbf --standard=Drupal

This follows on from efforts from @braders - there are not many
instances (although I kept the comment small) and we could fairly easily get rid of them
all and enable that rule in CI - which would lift the bar a little

@civibot
Copy link

civibot bot commented Jan 15, 2022

(Standard links)

@civibot civibot bot added the master label Jan 15, 2022
@@ -815,7 +815,7 @@ public static function isOrQuery() {
*
* @param int $cid
* @param int $oid
* @param "dupe-nondupe|nondupe-dupe" $oper
Copy link
Contributor

@braders braders Jan 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@param dupenondupe|nondupedupe $oper is defining that the param should be either a dupenondupe or nondupedupe class - not quite what was expected.

Currently the de-facto standards (https://docs.phpdoc.org/guide/references/phpdoc/types.html and https://github.com/php-fig/fig-standards/blob/master/proposed/phpdoc-tags.md only allow for @param string @oper which the available strings documented as a comment.

If prefered, we could do 'dupenondupe'|'nondupedupe' which whilst non-standard, is a format supported by at least one tool (https://psalm.dev/docs/annotating_code/typing_in_psalm/). There is also discussion of other syntaxes here, but unfortunately without much consensus phpDocumentor/phpDocumentor#557.

So probably sticking with @param string @oper for now is best.

(note this applies to the change in CRM/Core/BAO/CustomField.php too)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@braders yep that definiely seems to be the easier approach. I had it in mind that there was a way to list available strings that is agreed - I think the definitive place for standards is psr5 now - https://github.com/php-fig/fig-standards/blob/master/proposed/phpdoc.md - but I haven't read it through

@braders
Copy link
Contributor

braders commented Jan 15, 2022

+1 on enforcing capitalisation. That looks good.

@eileenmcnaughton eileenmcnaughton force-pushed the phpcs branch 2 times, most recently from c0791ca to b873d6d Compare January 16, 2022 02:34
This commit contains 14 files worth of changes from the following steps

1) install https://github.com/civicrm/coder.git
2) comment out <rule ref=Drupal.Commenting.FunctionComment.IncorrectParamVarName><severity>0</severity></rule>
3) run phpcbf --standard=Drupal

This follows on from efforts from @braders - there are not many
instances (although I kept the comment small) and we could fairly easily get rid of them
all and enable that rule in CI - which would lift the bar a little
@demeritcowboy demeritcowboy merged commit 1234dbf into civicrm:master Jan 16, 2022
@eileenmcnaughton eileenmcnaughton deleted the phpcs branch January 16, 2022 21:22
@eileenmcnaughton
Copy link
Contributor Author

thanks @braders @demeritcowboy

eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this pull request Jan 16, 2022
eileenmcnaughton added a commit to eileenmcnaughton/coder that referenced this pull request Jan 16, 2022
Per civicrm/civicrm-core#22515 there are a handful of these- we
can fix the bulk of them pretty easily
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants