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.

Sign up for GitHub

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

Jump to bottom

Changes of behavior detected in the tool between php74 and php80 #72

Closed
stronk7 opened this issue May 5, 2021 · 2 comments · Fixed by #73
Closed

Changes of behavior detected in the tool between php74 and php80 #72

stronk7 opened this issue May 5, 2021 · 2 comments · Fixed by #73

Comments

@stronk7
Copy link
Member

stronk7 commented May 5, 2021 •
edited
Loading

It has been detected that the information reported by the tool is different based on the php version used. Here it's a real case:

  • With PHP 7.4:
$ sudo port select php php74
Selecting 'php74' for 'php' succeeded. 'php74' is now active.
$ php local/moodlecheck/cli/moodlecheck.php -p=admin/tool/brickfield/classes/local/areas/mod_lesson/page_base.php -f=text
admin/tool/brickfield/classes/local/areas/mod_lesson/page_base.php
  • With PHP 8.0
$ sudo port select php php80
Selecting 'php80' for 'php' succeeded. 'php80' is now active.
$ php local/moodlecheck/cli/moodlecheck.php -p=admin/tool/brickfield/classes/local/areas/mod_lesson/page_base.php -f=text
admin/tool/brickfield/classes/local/areas/mod_lesson/page_base.php
    Line 40: Phpdocs for function page_base::find_relevant_areas has incomplete parameters list

This was first noticed with this GHA execution that is passing perfectly with PHP7, but reporting lots of errors with PHP8:

https://github.com/brickfield/moodle-tool_brickfield/runs/2510233810?check_suite_focus=true#step:11:1

Ciao :-)
@stronk7
Copy link
Member Author

stronk7 commented May 10, 2021

I'm debugging here a little bit the page_base::find_relevant_areas processing and this is quite curious:

  1. PHP 8 correctly returns param with type \core\event\base and name $event.

  2. PHP 7, instead, returns param with type base(without namespace) and name $event.

  3. local_moodlecheck_functionarguments() does strip namespaces from phpdoc information... so... at the end, the "incorrect" PHP 7 passes and the correct PHP 8 fails.

Still looking at which exact point the difference happens, surely it's some difference in the Tokenizer... and can imagine which the solution may be... but want to understand it....

@stronk7
Copy link
Member Author

stronk7 commented May 11, 2021

Aha,

mystery solved, that change came with PHP8 (treat namespaces as single token):

https://wiki.php.net/rfc/namespaced_names_as_token

So, I'm going to try to keep old behavior and, when php8 is detected, look for the last part of the namespace only.

Ciao :-)

stronk7 added a commit to stronk7/moodle-local_moodlecheck that referenced this issue May 11, 2021
@stronk7
Ensure that only the final part of namespaces is returned
b00c7b3 
This tool has limited namespace handling capabilities and
we always have been getting, exclusively, the last part of
every namespace detected in function params.

This was the default behavior for PHP7 tokenizer and, in fact,
when reading the phpdocs for those params... we are removing
everything but the final part of the namespace too.

But PHP8 changed the default behavior and its tokenizer now
returns the complete namespace as unique token.

https://wiki.php.net/rfc/namespaced_names_as_token

So, to keep everything working as it was... when we detect that
some type of the function params contains namespaces, we are also
going to remove everything but the final part of the namespace.

That matches previous behaviour 100%.

If some day we want to go processing full namespaces (instead of
their final parts), that will be great, but not now.

Fixes moodlehq#72.
This was referenced May 11, 2021
Merged
Closed
@snake snake closed this as completed in #73 May 12, 2021
@marinaglancy marinaglancy mentioned this issue May 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Ensure that only the final part of namespaces is returned stronk7/moodle-local_moodlecheck
1 participant
@stronk7