HTMLExecutingFunctions: Add more functions#437
Merged
rebeccahum merged 2 commits intodevelopfrom Jul 3, 2020
Merged
Conversation
david-binda
reviewed
Aug 19, 2019
Contributor
david-binda
left a comment
david-binda
left a comment
There was a problem hiding this comment.
Looks good to me! However, I left some minor feedback/questions inline.
|
|
||
| $prevPrevToken = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $prevToken - 1, null, true, null, true ); | ||
|
|
||
| if ( T_CLOSE_PARENTHESIS !== $this->tokens[ $prevPrevToken ]['code'] ) { |
Contributor
There was a problem hiding this comment.
I believe that we should also check whether the keyword we have found is a function call. As, for instance, the code, as it is now, would flag following code:
foo( variable ).appendTo;
|
|
||
| while ( $prevPrevToken > $parenthesis_opener ) { | ||
| $prevPrevToken = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $prevPrevToken - 1, null, true, null, true ); | ||
| if ( T_STRING === $this->tokens[ $prevPrevToken ]['code'] ) { // Contains a variable. |
Contributor
There was a problem hiding this comment.
I know that this code has been in place prior this PR, but I don't think that the // Contains a variable inline comment is true, as even a function call would match the T_STRING code. Eg.:
$( foo() ).appendTo;
Contributor
Author
|
Decided to prove to myself that these did indeed execute JavaScript: https://jsbin.com/pohefecucu/edit?html,js,output (creates 15 alerts that you'll need to click through!) There are some subtleties:
|
There are different ways to insert variables into the DOM, and our sniffs only covered some of them. The sniff now has an expanded list, which covers functions that have a syntax where the content is in the function arg, and also where the function arg is the target and the content is in the preceding method's arg. Fixes #435.
The extra unit test cases: - consider if the contents of a function was not a string but a function call (and update the inline comments to reflect that); this case was already correctly handled. - consider the case when the object that `appendTo()` (and similar) is being called in is a variable, and not a `$()` call (which we already check to see if it contains a simple string to avoid unnecessary violations) or other function call.
rebeccahum
approved these changes
Jul 3, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There are different ways to insert variables into the DOM, and our sniffs only covered some of them.
The sniff now has an expanded list, which covers functions that have a syntax where the content is in the function arg, and also where the function arg is the target and the content is in the preceding method's arg.
New functions:
afterappendTobeforeinsertAfterinsertBeforeprependprependToreplaceAllreplaceWithFixes #435.
Not included is the ability to handle
insertAdjacentHTML()or consolidating assignment toinnerHTML(andouterHTML) into this sniff.