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

php74: Fix a couple of remaining curly-bracket uses #72

Merged
merged 2 commits into from
Jun 10, 2020

Conversation

stronk7
Copy link
Member

@stronk7 stronk7 commented May 23, 2020

These escaped #65 and just noticed them today when executing
the bundled pear/PHP/scripts/phpcs binary (not reproducible
from web or tests).

Also fix an unrelated typo in CHANGES.md

Finally, have added some important changes to travis, aiming for simplicity (include jobs instead of matrix + excludes), better nodejs handling...

@stronk7
Copy link
Member Author

stronk7 commented May 23, 2020

Can be checked with:

ag '(\$[0-9a-zA-Z_]+|\))(\[['\''"\w$]+\])?{[^\n=}]+}' --php | sort | uniq

(note that there are 2 cases within PHPCompatibility, but they are literals there, not code)

Or running, with php74:

local/codechecker/pear/PHP/scripts/phpcs --standard=local/codechecker/moodle mod/label/classes

Without the patch there are some PHP notices for those lines. With the patch it runs ok.

These escaped moodlehq#65 and just noticed them today when executing
the bundled pear/PHP/scripts/phpcs binary (not reproducible
from web or tests).

Also fix an unrelated typo in CHANGES.md
@stronk7 stronk7 force-pushed the curly_brackets_take_two branch 9 times, most recently from 132224b to 807c962 Compare May 24, 2020 09:18
- Change from matrix - excludes to includes.
- Add mising details.
- Add support for dynamic node version.
- Cache .nvm dir
Copy link
Contributor

@andrewnicols andrewnicols left a comment

Choose a reason for hiding this comment

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

We shouldn't be specifying a version here.

All supported versions of Moodle since 3.5 have the nvmrc, and we do something like:

 if [ -f .nvmrc ]; then
    nvm install
    nvm use
else
    // Old LTS before we had an nvmrc is lts/carbon
    nvm install lts/carbon
fi

@stronk7
Copy link
Member Author

stronk7 commented May 24, 2020

We shouldn't be specifying a version here.

All supported versions of Moodle since 3.5 have the nvmrc, and we do something like:

Yeah, I'm well aware and, in fact, that was my primary attempt, to leave 35_STABLE and up just using nvm use && nvm_install. But it didn't work, I did a good number of attempts against travis, but I'm afraid:

  1. nvm is installed BEFORE core codebase is available thanks to moodle-plugin-ci install. So, at all effects, it's not .nvmrc aware.
  2. So, then I tried moving the nvm stuff to after moodle-plugin-ci install but it also failed (because moodle-plugin-ci install needs a node/npm available).
  3. So, lalala, only solution was to, explicitly, set the default (14).

To make this work as expected, once again, changes to moodle-plugin-ci are required. :-(

Ciao :-)

@stronk7 stronk7 requested a review from andrewnicols May 24, 2020 17:33
@stronk7
Copy link
Member Author

stronk7 commented Jun 9, 2020

Ping!

@andrewnicols andrewnicols merged commit d485c3a into moodlehq:master Jun 10, 2020
@andrewnicols
Copy link
Contributor

Sorry - this had dropped off my radar. Thanks for the detailed explanation.

@stronk7
Copy link
Member Author

stronk7 commented Jun 10, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants