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

PHPCompatibility #205

Closed
mackensen opened this issue Feb 24, 2023 · 4 comments · Fixed by #206
Closed

PHPCompatibility #205

mackensen opened this issue Feb 24, 2023 · 4 comments · Fixed by #206

Comments

@mackensen
Copy link

I may just be misunderstanding something about the PHPCompatibility checker. I added is_countable() on https://github.com/LafColITS/moodle-local_ldap/tree/countable to deal with some PHP 8.0 wonkiness. That code is also tested against PHP 7.1, so it ought to fail, and doesn't. I assume the PHPUnit test succeeds because of a polyfill in the dev dependency, but I wouldn't think that would affect static analysis.

@jrchamp
Copy link
Contributor

jrchamp commented Feb 27, 2023

Hi @mackensen,

When I run the PHPCompatibility ruleset directly against moodle-local_ldap's countable branch, is_countable shows up for me along with several other issues.

FILE: locallib.php
----------------------------------------------------------------------------------------------------------------------------------------------
FOUND 18 ERRORS AFFECTING 18 LINES
----------------------------------------------------------------------------------------------------------------------------------------------
 152 | ERROR | Function ldap_control_paged_result() is deprecated since PHP 7.4 and removed since PHP 8.0; Use ldap_search() instead
 156 | ERROR | The constant "LDAP_CONTROL_PAGEDRESULTS" is not present in PHP version 7.2 or earlier
 168 | ERROR | The function ldap_search() does not have a parameter "controls" in PHP version 7.2 or earlier
 176 | ERROR | The function ldap_list() does not have a parameter "controls" in PHP version 7.2 or earlier
 186 | ERROR | Function ldap_control_paged_result_response() is deprecated since PHP 7.4 and removed since PHP 8.0; Use ldap_search() instead
 191 | ERROR | The function ldap_parse_result() does not have a parameter "controls" in PHP version 7.2 or earlier
 193 | ERROR | The constant "LDAP_CONTROL_PAGEDRESULTS" is not present in PHP version 7.2 or earlier
 194 | ERROR | The constant "LDAP_CONTROL_PAGEDRESULTS" is not present in PHP version 7.2 or earlier
 261 | ERROR | The function is_countable() is not present in PHP version 7.2 or earlier
 559 | ERROR | Function ldap_control_paged_result() is deprecated since PHP 7.4 and removed since PHP 8.0; Use ldap_search() instead
 563 | ERROR | The constant "LDAP_CONTROL_PAGEDRESULTS" is not present in PHP version 7.2 or earlier
 576 | ERROR | The function ldap_search() does not have a parameter "controls" in PHP version 7.2 or earlier
 586 | ERROR | The function ldap_list() does not have a parameter "controls" in PHP version 7.2 or earlier
 600 | ERROR | Function ldap_control_paged_result_response() is deprecated since PHP 7.4 and removed since PHP 8.0; Use ldap_search() instead
 603 | ERROR | The function ldap_parse_result() does not have a parameter "controls" in PHP version 7.2 or earlier
 605 | ERROR | The constant "LDAP_CONTROL_PAGEDRESULTS" is not present in PHP version 7.2 or earlier
 606 | ERROR | The constant "LDAP_CONTROL_PAGEDRESULTS" is not present in PHP version 7.2 or earlier
 774 | ERROR | The function is_countable() is not present in PHP version 7.2 or earlier
----------------------------------------------------------------------------------------------------------------------------------------------


FILE: tests/sync_test.php
----------------------------------------------------------------------------------------------------------------------------------------------
FOUND 12 ERRORS AFFECTING 12 LINES
----------------------------------------------------------------------------------------------------------------------------------------------
 664 | ERROR | Function ldap_control_paged_result() is deprecated since PHP 7.4 and removed since PHP 8.0; Use ldap_search() instead
 668 | ERROR | The constant "LDAP_CONTROL_PAGEDRESULTS" is not present in PHP version 7.2 or earlier
 688 | ERROR | Function ldap_control_paged_result_response() is deprecated since PHP 7.4 and removed since PHP 8.0; Use ldap_search() instead
 693 | ERROR | The function ldap_parse_result() does not have a parameter "controls" in PHP version 7.2 or earlier
 695 | ERROR | The constant "LDAP_CONTROL_PAGEDRESULTS" is not present in PHP version 7.2 or earlier
 696 | ERROR | The constant "LDAP_CONTROL_PAGEDRESULTS" is not present in PHP version 7.2 or earlier
 718 | ERROR | Function ldap_control_paged_result() is deprecated since PHP 7.4 and removed since PHP 8.0; Use ldap_search() instead
 722 | ERROR | The constant "LDAP_CONTROL_PAGEDRESULTS" is not present in PHP version 7.2 or earlier
 742 | ERROR | Function ldap_control_paged_result_response() is deprecated since PHP 7.4 and removed since PHP 8.0; Use ldap_search() instead
 747 | ERROR | The function ldap_parse_result() does not have a parameter "controls" in PHP version 7.2 or earlier
 749 | ERROR | The constant "LDAP_CONTROL_PAGEDRESULTS" is not present in PHP version 7.2 or earlier
 750 | ERROR | The constant "LDAP_CONTROL_PAGEDRESULTS" is not present in PHP version 7.2 or earlier
----------------------------------------------------------------------------------------------------------------------------------------------

@jrchamp
Copy link
Contributor

jrchamp commented Feb 27, 2023

If possible, I recommend using the moodle-plugin-ci as part of your GitHub actions: https://github.com/moodlehq/moodle-plugin-ci/blob/master/gha.dist.yml

You'll probably need to customize it a bit to make sure you are testing the appropriate combinations. Here's the version we're using for the moodle-mod_zoom plugin: https://github.com/ncstate-delta/moodle-mod_zoom/blob/main/.github/workflows/ci.yml

@mackensen
Copy link
Author

@jrchamp Thanks for those sample results, that's helpful. I haven't implemented GHA for this plugin (yet) because I haven't come up with a good solution for the OpenLDAP support (I need to able to load the custom eduPerson schema). I use GHA for all my other plugins and like it.

I realized on reviewing that I'd excluded locallib.php from code-checker at some point in the past. I don't remember why. Removing that means I get results now, though oddly enough still not the right ones.

This is a run for PHP 7.1, but it looks like PHP 8.0 results: https://app.travis-ci.com/github/LafColITS/moodle-local_ldap/jobs/596885638. I guess that's a function of moodlehq/moodle-local_codechecker#109.

@jrchamp
Copy link
Contributor

jrchamp commented Feb 27, 2023

Yeah, it looks like moodle-plugin-ci might not be setting a testVersion value, which seems to prevent PHPCompatibility from showing new functions/features.

To get the most out of the PHPCompatibility standard, you should specify a testVersion to check against. That will enable the checks for both deprecated/removed PHP features as well as the detection of code using new PHP features.

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 a pull request may close this issue.

2 participants