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

[Tools] Enable PHPCS for fix_timepoint_date_problems.php #5984

Closed
wants to merge 3 commits into from
Closed

[Tools] Enable PHPCS for fix_timepoint_date_problems.php #5984

wants to merge 3 commits into from

Conversation

johnsaigle
Copy link
Contributor

No description provided.

@johnsaigle johnsaigle added the Testing PR contains test plan or automated test code (or config files for Travis) label Jan 29, 2020
@johnsaigle johnsaigle added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Feb 6, 2020
@stale
Copy link

stale bot commented Mar 29, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the State: Stale PR that has had no recent activity, needs to be triaged for review or closure to proceed label Mar 29, 2020
@laemtl laemtl removed State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) State: Stale PR that has had no recent activity, needs to be triaged for review or closure to proceed labels Jun 17, 2020
@laemtl
Copy link
Contributor

laemtl commented Jun 17, 2020

@driusan This is ready for review

@laemtl laemtl self-assigned this Jun 17, 2020
@driusan driusan closed this Jun 17, 2020
@driusan driusan reopened this Jun 17, 2020
@laemtl laemtl self-requested a review June 18, 2020 01:00
@ridz1208
Copy link
Collaborator

@laemtl just to be sure, when you are approving these are you also testing them ?

@laemtl
Copy link
Contributor

laemtl commented Jun 18, 2020

@ridz1208 I rebased the code and I made sure PHPCS does not detect any problem.
I did not test the script itself though as the changes are done to comply with PHPCS formatting rules - but can if necessary.

@laemtl laemtl self-requested a review June 18, 2020 18:07
@laemtl
Copy link
Contributor

laemtl commented Jun 18, 2020

I tested the script and noticed a Fatal error:
PHP Fatal error: Uncaught TypeError: Return value of TimePoint::getScreeningStatus() must be of the type string, null returned in /var/www/html/loris/php/libraries/TimePoint.class.inc:493

Should the return type of php/libraries/TimePoint.class.inc:getScreeningStatus() changed for ?string?

@ridz1208
Copy link
Collaborator

@laemtl yes please change it

@johnsaigle johnsaigle added the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Jun 22, 2020
@laemtl laemtl removed the State: Needs rebase PR that needs to be rebased to proceed (conflicts, wrong branch...) label Jun 25, 2020
* display and stop processing if action=help
*/
if (empty($argv[1]) || $argv[1] == 'help') {
fwrite(STDERR, "Usage: \n\n");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is changing the destination of the error message. It should still be going to stderr if the usage was incorrect.

* display and stop processing if action=help
*/
if (empty($argv[1]) || $argv[1] == 'help') {
//phpcs:disable
Copy link
Collaborator

Choose a reason for hiding this comment

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

👎

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We either have to disable PHPCS here or modify the help content so that it's ahrder to read and maintain. Since it's only disabled for this heredoc I think it's a fine thing to ignore. This is a section of the code where PHPCS is not helpful.

@laemtl
Copy link
Contributor

laemtl commented Jun 30, 2020

Replaced by #6798

@laemtl laemtl closed this Jun 30, 2020
driusan pushed a commit that referenced this pull request Mar 8, 2021
AlexandraLivadas pushed a commit to AlexandraLivadas/Loris that referenced this pull request Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Testing PR contains test plan or automated test code (or config files for Travis)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants