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

CONTRIB-8920 moodle-cs: Deprecation alerts for capabilities #3

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 111 additions & 0 deletions moodle/Sniffs/Access/DeprecatedCapabilitySniff.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
<?php
// This file is part of Moodle - http://moodle.org/
//
// Moodle is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// Moodle is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.
//
// You should have received a copy of the GNU General Public License
// along with Moodle. If not, see <http://www.gnu.org/licenses/>.

/**
* Checks that each file contains necessary login checks.
*
* @package local_codechecker
* @copyright 2022 Laurent David <laurent.david@moodle.com>
* @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
*/

namespace MoodleCodeSniffer\moodle\Sniffs\Access;

use PHP_CodeSniffer\Files\File;
use PHP_CodeSniffer\Sniffs\Sniff;

class DeprecatedCapabilitySniff implements Sniff {

// phpcs:disable moodle.NamingConventions.ValidVariableName.MemberNameUnderscore
/**
* If we try to check this capability, a warning will be shown.
*
* @var array
*/
public $capabilitiesWarningList = [];

/**
* If we try to check this capability, an error will be shown.
*
* @var array
*/
public $capabilitiesErrorList = [];
Comment on lines +38 to +45
Copy link
Contributor

Choose a reason for hiding this comment

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

How are these filled?


// phpcs:enable

/**
* Access function type
*
* @var string[]
*/
public $accessfunctions = ['has_access', 'has_capability'];
Copy link
Contributor

Choose a reason for hiding this comment

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

What about has_any_capability, has_all_capabilities, and the require_* versions?


/**
* Register for open tag (only process once per file).
*/
public function register() {
return array(T_OPEN_PARENTHESIS);
}

/**
* Processes php files and for required login checks if includeing config.php
*
* @param File $file The file being scanned.
* @param int $stackptr The position in the stack.
*/
public function process(File $file, $stackptr) {
$functionnamepos = $file->findPrevious(T_STRING, $stackptr - 1);
if ($functionnamepos === false) {
return;
}
$tokens = $file->getTokens();
if (empty($tokens[$functionnamepos]) || !$this->is_an_access_function($tokens[$functionnamepos])) {
return;
}

$alldeprecated = array_merge($this->capabilitiesErrorList, $this->capabilitiesWarningList);
$closeparenthesispos = $file->findNext(T_CLOSE_PARENTHESIS, $stackptr + 1);
if ($closeparenthesispos === false) {
// Live coding, parse error or not a function call.
return;
}
$closeparenthesistoken = $tokens[$closeparenthesispos];
$starttoken = $closeparenthesistoken['parenthesis_opener'] + 1;
$endtoken = $closeparenthesistoken['parenthesis_closer'];
$values = $file->getTokensAsString($starttoken, $endtoken - $starttoken);
foreach ($alldeprecated as $capability) {
if (strpos($values, $capability) !== false) {
if (in_array($capability, $this->capabilitiesWarningList)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (in_array($capability, $this->capabilitiesWarningList)) {
if (in_array($capability, $this->capabilitiesWarningList, true)) {

$file->addWarning("The following capability '$capability' will be deprecated soon.", $starttoken,
'DeprecatedCapability');
} else {
$file->addError("The following capability '$capability' has been deprecated.", $starttoken,
'DeprecatedCapability');
}
}
}
}

/**
* Is the current name an access function
*
* @param array $token current token
* @return bool true if the current methodname is access function.
*/
protected function is_an_access_function(array $token) {
return in_array($token['content'], $this->accessfunctions);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return in_array($token['content'], $this->accessfunctions);
return in_array($token['content'], $this->accessfunctions, true);

Copy link
Author

Choose a reason for hiding this comment

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

Hi @jrchamp

Thanks for the quick feedback. I went back to the doc of in_array I have not used this parameter before, so good to know. I could probably have used isset also. I am not sure which one is quicker.
I will modify this once you will have a chance to look at the rest. The global approach needs to be validated and the big picture is: do we really get this the capabilitiesWarningList through rulseset.xml or by reading each access.php.

Thanks,

Laurent

Copy link
Contributor

@jrchamp jrchamp May 24, 2022

Choose a reason for hiding this comment

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

Glad it was useful! I'm just a community member, so HQ will review the rest of the code.

isset() is quicker, but would require array keys instead of array values. ['a' => true, 'b' => true] Overall, in_array() is probably fine for this use case because O(log(n)) vs O(n) isn't a huge difference when n is small.

Side note: As long as you aren't allowing meaningful null values, isset($a[$b]) is the faster replacement for array_key_exists($b, $a)

Copy link
Author

Choose a reason for hiding this comment

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

Hi @jrchamp,

Cheers, I will take that into account once the full review is done. As we say "Make it work, Make it better"...

To give you the context will have couple of capabilities in the array so (n) will be small and as per the second in_array, the number of parameters is also very small (2 or 3 max). I knew about the isset better performances, but then again if performance is not the main issue (and here we have only a couple of elements), I prefer in_array as it is more understandable as you read the code.
I have never noticed with the third parameter of is_array thanks for the input and the detailed explanations !

Laurent

}
}
9 changes: 9 additions & 0 deletions moodle/ruleset.xml
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,15 @@
<severity>0</severity>
</rule>

<!-- Setup deprecated capability list -->
<rule ref="moodle.Access.DeprecatedCapability">
<properties>
<!-- <property name="capabilitiesWarningList" type="array">-->
<!-- <element value="moodle/course:useremail"/>-->
<!-- </property>-->
</properties>
</rule>

<!-- Let's add the complete PHPCompatibility standard -->
<rule ref="PHPCompatibility" />

Expand Down
8 changes: 8 additions & 0 deletions moodle/tests/fixtures/moodle_access.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?php
defined('MOODLE_INTERNAL') || die(); // Make this always the 1st line in all CS fixtures.
$context = context_system::instance();
// phpcs:set moodle.Access.DeprecatedCapability capabilitiesWarningList[] moodle/course:useremail
// phpcs:set moodle.Access.DeprecatedCapability capabilitiesErrorList[] moodle/site:useremail
has_capability('moodle/course:useremail', $context);
has_capability('moodle/site:useremail', $context);

25 changes: 25 additions & 0 deletions moodle/tests/moodlestandard_test.php
Original file line number Diff line number Diff line change
Expand Up @@ -956,4 +956,29 @@ public function test_moodle_files_requirelogin_nomoodlecookies_ok() {

$this->verify_cs_results();
}

/**
* Test the moodle.Access.DeprecatedCapabilitySniff sniff.
*
*
* @covers \MoodleCodeSniffer\moodle\Sniffs\Access\DeprecatedCapabilitySniff
*/
public function test_moodle_deprecated_access() {

// Define the standard, sniff and fixture to use.
$this->set_standard('moodle');
$this->set_sniff('moodle.Access.DeprecatedCapability');
$this->set_fixture(__DIR__ . '/fixtures/moodle_access.php');
// Define expected results (errors and warnings). Format, array of:
// - line => problem.
$this->set_errors([
7 => 'The following capability \'moodle/site:useremail\' has been deprecated',
]);
$this->set_warnings([
6 => 'The following capability \'moodle/course:useremail\' will be deprecated soon',
]);

// Let's do all the hard work!
$this->verify_cs_results();
}
}