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 Grant::retrieve to meet full phpcs standard #22558

Merged
merged 1 commit into from
Jan 24, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Fix Grant::retrieve to meet full phpcs standard

Before

After applying #22543 I find the changes don't meet the full drupal standard

After

The new boilerplate meets the full drupal standard

Technical Details

The reason our standard falls short of 'the full drupal' it that it was too big a lift to retrofit it all at once. We have been slowly raising the bar. Since we are basically creating new boilerplate I was concerned we were actually going backwards against 'the full drupal' so I worked through it on this file to see what a fully compliant bit of boilerplate would look like

Comments

@civibot
Copy link

civibot bot commented Jan 18, 2022

(Standard links)

@colemanw
Copy link
Member

OK. Thanks for working through that. @eileenmcnaughton
The only thing I don't like is the verbosity considering this is a deprecated boilerplate function can we get rid of the lines describing the variables and the line describing the return value?

@eileenmcnaughton
Copy link
Contributor Author

@colemanw those lines are part of the standard - we are likely to be stuck with the boilerplate for a long time so it's probably better to meet the standard

@colemanw
Copy link
Member

So is the standard that every @param and every @return needs a description below it? Does that mean you need to add a description below @return in the commonRetrieve function?

@colemanw
Copy link
Member

retest this please

@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton
Copy link
Contributor Author

@colemanw yeah I guess so - I ran coder with all of these lines removed

https://github.com/civicrm/coder/blob/8.x-2.x-civi/coder_sniffer/Drupal/ruleset.xml#L11-L71

@colemanw colemanw merged commit a71379d into civicrm:master Jan 24, 2022
@colemanw colemanw deleted the coleman branch January 24, 2022 00:31
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.

2 participants