search

wp-cli / embed-command

Inspects oEmbed providers, clears embed cache, and more.
MIT License
7 stars 11 forks source link

Apply PHPCS `WP_CLI_CS` rules #48

Closed pattonwebz closed 5 years ago

pattonwebz commented 5 years ago

This applies some changes to bring the code in line with the new WPCliCS ruleset.

There are only 2 remaining WARNINGs in this package at the time when I open this PR. Some advice about if I should silence the warnings. The 2 items left are:

williampatton@williampatton-ws:~/wd-sw/embed-command$ phpcs --standard=WPCliCS src/*

FILE: /home/williampatton/wd-sw/embed-command/src/Cache_Command.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
 116 | WARNING | serialize() found. Serialized data has known vulnerability
     |         | problems with Object Injection. JSON is generally a better
     |         | approach for serializing data. See
     |         | https://www.owasp.org/index.php/PHP_Object_Injection
--------------------------------------------------------------------------------

FILE: /home/williampatton/wd-sw/embed-command/src/Fetch_Command.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
 237 | WARNING | Method name "_oembed_create_xml" should not be prefixed with
     |         | an underscore to indicate visibility
--------------------------------------------------------------------------------

Time: 341ms; Memory: 14MB

The _oembed_create_xml method I guess I should silence but what about the use of serialize()?

This is part 2 of #45

Related https://github.com/wp-cli/wp-cli/issues/5179

pattonwebz commented 5 years ago

@schlessera @jrfnl If you could look at this one and give me any feedback about it that would be good. Also advice about if I should silence the 2 remaining warnings.

And if you could confirm that the PR: #47 is matching what is expected that'd be awesome. If this is the way to go about this I will be able to look at more of the command repos tomorrow and do the same thing in those.

pattonwebz commented 5 years ago

The only thing flagged in this repo now is:

williampatton@williampatton-ws:~/git-repos/wpclicmd/embed-command$ phpcs --standard=WPCliCS ./

FILE: /home/williampatton/git-repos/wpclicmd/embed-command/src/Fetch_Command.php
--------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------
 237 | WARNING | Method name "_oembed_create_xml" should not be prefixed with
     |         | an underscore to indicate visibility
--------------------------------------------------------------------------------

Time: 343ms; Memory: 12MB

@schlessera this final one will be your decision about what to do here. Whitelist or fix.

schlessera commented 5 years ago

I think you can safely rename the _oembed_create_xml method to just remove the underscore. It is used in two places within the FetchCommand only.

pattonwebz commented 5 years ago

This PR should be ready for checking. The ruleset is added in #47 and after that is merged the PHPCS checks should pass on this PR.

There is still a non-related issue causing the unit tests to fail though. I have not investigated that fail however it was present in the build prior to the updates added in the 2 CS changesets.

schlessera commented 5 years ago

@pattonwebz Regarding the failing test, this should be fixed via https://github.com/wp-cli/embed-command/pull/50. As soon as that one is merged, you can get your tests passing by pulling in latest master.

schlessera commented 5 years ago

@pattonwebz https://github.com/wp-cli/embed-command/pull/50 was merged, can you merge in latest master here?