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

DontExtractStandard.xml file creation #2456

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

DontExtractStandard.xml file creation #2456

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1a08be5
DontExtractStandard.xml file creation
aiolachiara Jun 13, 2024
c9c9d3d
Update DontExtractStandard.xml
aiolachiara Jun 19, 2024
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
33 changes: 33 additions & 0 deletions WordPress/Docs/PHP/DontExtractStandard.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
<?xml version="1.0"?>
<documentation xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:noNamespaceSchemaLocation="https://phpcsstandards.github.io/PHPCSDevTools/phpcsdocs.xsd"
title="Don't exctract"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
title="Don't exctract"
title="Don't extract"

>
<standard>
<![CDATA[
Restricts the usage of `extract()` php function. It makes code harder to debug and harder to understand and may cause conflicts on variables names.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Restricts the usage of `extract()` php function. It makes code harder to debug and harder to understand and may cause conflicts on variables names.
Forbids the usage of the PHP native `extract()` function. Using `extract()` makes code harder to debug, harder to understand and may cause unexpected behaviour when variables names conflict.

]]>
</standard>
<code_comparison>
<code title="Valid: Similarly named functions or methods are fine">
<![CDATA[
$var_array = array(
"title" => "My title",
"content" => "My content",
"ID" => 123
);
my_extract( $var_array, EXTR_OVERWRITE );
Comment on lines +12 to +19
Copy link
Member

Choose a reason for hiding this comment

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

Rather than giving a valid example as being "Use a similarly name function", I'd suggest demonstrating how to use keys from the $var_array directly. I'd also suggest renaming the variable to something a little more contextual.

$post_data = array(
    'title'   => 'My title',
    'content' => 'My content',
    'ID'      => 123
);
<em>echo $post_data['title'];</em>

The <em> </em> is used here as that's the bit we're emphasising. It should be added around the extract() call in the Invalid section too.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @GaryJones' remark here. The "Valid" example should show the correct way of doing it, not what's "tolerated" to prevent false positives.

When updating the code sample, please also update the "Valid" description.

]]>
</code>
<code title="Invalid: ">
Copy link
Member

Choose a reason for hiding this comment

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

This line seems incomplete?

Copy link
Author

Choose a reason for hiding this comment

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

you're right, I fixed the description of invalid code sample

<![CDATA[
$var_array = array(
"title" => "My title",
"content" => "My content",
"ID" => 123
);
Comment on lines +24 to +28
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$var_array = array(
"title" => "My title",
"content" => "My content",
"ID" => 123
);
$var_array = array(
'title' => 'My title',
'content' => 'My content',
'ID' => 123
);

The code samples should comply with WPCS (aside from the issue being demonstrated). WPCS would flag the unnecessary use of double quotes here.

extract( $var_array, EXTR_OVERWRITE );
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
extract( $var_array, EXTR_OVERWRITE );
extract( $var_array );

The second parameter is optional and rarely passed, so no need to include it in the example.

Another reason to remove it, is that it could be confusing to people - "but the example says it is only forbidden when used with EXTR_OVERWRITE...".

]]>
</code>
</code_comparison>
</documentation>
Loading