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

Escape instead of removing <script> tag contents #5

Merged
merged 3 commits into from
Jul 10, 2017
Merged

Conversation

thiemowmde
Copy link
Contributor

No description provided.

@thiemowmde thiemowmde requested a review from bekh6ex July 5, 2017 11:10
Copy link
Contributor Author

@thiemowmde thiemowmde left a comment

Choose a reason for hiding this comment

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

I wonder why the bracket was moved. The rest of the code aligns brackets vertically, and this is what I did for consistency. But if you think we should start changing this I'm fine with that. Bot additional commits look fine for me. Feel free to merge.

</div>";

assertThat($html, is(htmlPiece(havingChild(
both(withTagName('script'))
->andAlso(havingTextContents(HtmlMatcher::SCRIPT_BODY_REPLACEMENT))))));
->andAlso(havingTextContents("<span><\\/span>"))))));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be '<span><\/span>' now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder why the bracket was moved.

I thought it was a mistake. It just looks weird - single bracket on the line.
Speaking of other methods, I believe there is quiet a difference between methods and closures. The last serve to shorten the code so it would be nice to format them as short as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What you say confuses me a bit. Shouldn't code style be consistent? Either always align { and } vertically (which is, as a side note, the style I personally preferred before I started doing MediaWiki professionally). Or always have the opening { at the end of the line (which is the MediaWiki standard).

If we can agree on using the MediaWiki code style in this repository this will resolve itself. :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's the same problem I've already noted at mediawiki phpcs Phabricator board:

// shortest
$prefixed = array_map(function ($i) { return 'x' . $i; }, $array);

// mediawiki style
$prefixed = array_map(function ($i) { 
    return 'x' . $i; 
}, $array);

// consistent in terms of argument list wrapping
$prefixed = array_map(
    function ($i) { 
        return 'x' . $i; 
    }, 
    $array
);

The last is the most consistent in terms of formatting, the first is the shortest one, the middle is what is used in mediawiki style.
As soon as there is no complex stuff happening in closure (as in example) and it doesn't hit width limit, I would prefer the first style because it is readable and "concentrated".
I think this kind of decision is up to a human and should not be enforced by style checker.
As well as this case, for example:

assertThat($html, is(htmlPiece(havingChild(
    both(withTagName('script'))
        ->andAlso(havingTextContents("<span><\\/span>"))))));
// VS more consistent
assertThat(
    $html,
    is(
        htmlPiece(
            havingChild(
                both(withTagName('script'))->andAlso(havingTextContents("<span><\\/span>"))
            )
        )
    )
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The last is the most consistent in terms of formatting […] the middle is what is used in mediawiki style.

I'm afraid I don't get the problem. Two of the three examples you gave are allowed when using the MediaWiki code style, including the one you refer to as "most consistent".

[…] I would prefer the first style because it is readable […]

Sorry, but no. This is as bad as a single-line if ( … ) return …;. Significant control structures like a return … should always be on their own line.

As well as this case, for example: […]

Sorry, but I don't understand which one is causing problems and why.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm afraid I don't get the problem. Two of the three examples you gave are allowed when using the MediaWiki code style, including the one you refer to as "most consistent".

With middle style it is hard to track arguments position. Last one takes too much vertical space for this simple task.

Significant control structures like a return … should always be on their own line.

  1. Quiet bold statement. Why? In many projects if ( … ) return …; structure considered fine.
  2. Then what about PHP RFC: Arrow Functions which introduces fn($x) => $arr[$x] syntax with implicit return? I suppose those should not be allowed at all then?

Sorry, but I don't understand which one is causing problems and why.

I'm talking about consistency in indentation. We can have a strict rule that each closing bracket goes on a new line, but then we end up with expressions using too much of a vertical space which doesn't bring any readability, I think it even makes it worse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everything you wrote sounds like you are afraid of vertical space. Oh dear. Can you try to disable a few toolbars and pinned windows and use the gained space to create code that exposes it's structure a little more obviously? I notice for a while now that you tend to cram multiple things into one line. I, personally, try to not do this any more since I am not coding on 25 rows text mode screens any more.

@bekh6ex bekh6ex merged commit 5a9b465 into master Jul 10, 2017
@bekh6ex bekh6ex deleted the doNotStripScript branch July 10, 2017 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants