Repository navigation
Conversation
|
Just documenting here that I chatted in person with @tikifez and he will add valid and invalid examples. No need to review this PR yet. |
|
Added code comparison with valid and invalid examples. |
1 similar comment
|
Added code comparison with valid and invalid examples. |
rodrigoprimo
left a comment
rodrigoprimo
left a comment
There was a problem hiding this comment.
Thanks for working on this PR, @tikifez! I'm not a maintainer of the project so take my review with a grain of salt. That being said, I left some comments based on my own experience creating PHPCS and WPCS sniffs.
Also, please make sure to include a reference to the parent issue as mentioned in the description of #1722:
Please mention this issue in your pull request description Related to #1722 so your pull request will show up in this issue.
| title="Preg Quote Delimiter" | ||
| > | ||
|
|
||
There was a problem hiding this comment.
should be aligned with
Here and in the other places where is used.
| Passing the $delimiter parameter to preg_quote() is strongly recommended to ensure the delimiter required by the PCRE functions are escaped. | ||
| ]]> | ||
|
|
There was a problem hiding this comment.
This newline is not necessary.
There was a problem hiding this comment.
Removed unnecessary newlines.
| > | ||
|
|
||
| Passing the $delimiter parameter to preg_quote() is strongly recommended to ensure the delimiter required by the PCRE functions are escaped. |
There was a problem hiding this comment.
to ensure the delimiter required by the PCRE functions are escaped.
I haven't looked deeply into this so I might be missing something, but I'm not sure this is the reason to recommend that the $delimiter parameter is passed.
I got this from the issue where this sniff was introduced:
Verify that the optional second parameter $delimiter of preg_quote() is always passed.
Too often I come across code where it is missing and unless it is passed, the default / delimiters are presumed, which is often wrong.
There was a problem hiding this comment.
Updated the description and changed the example valid code to be one from WordPress core.
| ]]> | ||
| No newline at end of file |
There was a problem hiding this comment.
Add a clear line at the end of the file please.
|
|
||
|
||
| preg_quote( $dir, '#' ) |
There was a problem hiding this comment.
| preg_quote( $dir, '#' ) | |
| preg_quote( $dir, <em>'#'em> ) |
Use the ... to highlight the focus of the code snippet.
|
@tikifez, I was just wondering if you'll have a chance to finish this off in the near future. It would be great if this PR could be included in the next WPCS release. If you haven't got time or lost interest, please let us know and we'll see if we can find someone to take over. Thanks! |
|
@tikifez, please let us know within a week if you are still interested in finishing this PR. If we don't hear back from you, we will presume you don't have time, and we will see if we can find someone else to take over and finish it. Thanks for your work so far! |
|
Closing as superseded by merged PR #2677 |
4 participants
Related to #1722