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

DontExtractStandard.xml file creation #2456

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

Conversation

aiolachiara
Copy link

@aiolachiara aiolachiara commented Jun 13, 2024

Related to #1722

my_extract( $var_array, EXTR_OVERWRITE );
]]>
</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

fix incomplete description of invalid use case description
Comment on lines +12 to +19
<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 );
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.

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

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

@aiolachiara Thank you for working on this!

I agree with the remark @GaryJones left previously and have added a few more nitpick comments inline.

I look forward to seeing the next iteration of the PR!

<?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.

"content" => "My content",
"ID" => 123
);
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...".

Comment on lines +24 to +28
$var_array = array(
"title" => "My title",
"content" => "My content",
"ID" => 123
);
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.

my_extract( $var_array, EXTR_OVERWRITE );
]]>
</code>
<code title="Invalid: use `extract()` function">
Copy link
Member

@jrfnl jrfnl Jul 22, 2024

Choose a reason for hiding this comment

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

Suggested change
<code title="Invalid: use `extract()` function">
<code title="Invalid: Using the `extract()` function.">

Comment on lines +12 to +19
<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 );
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.

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

Successfully merging this pull request may close these issues.

3 participants