Opened 5 years ago
Last modified 5 years ago
#49059 assigned enhancement
Whitespace inside p element in wp-signup.php should be removed
Reported by: | henry.wright | Owned by: | |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | 3.0 |
Component: | Networks and Sites | Keywords: | has-patch dev-feedback |
Focuses: | multisite, coding-standards | Cc: |
Description
In wp-signup.php there is a paragraph of text with a lot of left and right whitespace.
<p> Welcome back, Peter. By filling out the form below, you can <strong>add another site to your account</strong>. There is no limit to the number of sites you can have, so create to your heart's content, but write responsibly!. </p>
We should remove the whitespace because it isn't necessary.
Attachments (3)
Change History (36)
#1
@
5 years ago
- Keywords commit added
- Milestone changed from Awaiting Review to 5.4
- Owner set to johnjamesjacoby
- Status changed from new to assigned
- Version set to 3.0
#2
@
5 years ago
Hi there, thanks for the ticket!
This whitespace in signup_another_blog() doesn't seem much different from similar <h2>
and <p>
tags in confirm_another_blog_signup()
, signup_user()
, confirm_user_signup()
, confirm_blog_signup()
in the same file, and probably a lot other files in core.
We could move some of it inside the PHP tags, but I'm not sure it's worth the effort.
Is there a problem this particular instance is causing? Browsers should ignore the extra whitespace.
#3
follow-ups:
↓ 4
↓ 9
@
5 years ago
@SergeyBiryukov you are correct there are other instances in core where this happens. Although minor, I have for years always thought them to be worth correcting.
The problem I have always seen with them is the markup they produce is oddly difficult to follow when viewing source.
While modern tooling and browser inspectors have helped improve that experience, I see no harm in removing some instances of jumping in and out of <?php
simply to output a wrapping HTML Tag when so many other places in the same file are already concatenating them.
I'd prefer to clean this part of the house up, rather than sweep it under the rug.
I don't think every instance you've brought up needs a separate ticket. I/we could normalize all of them here.
The hidden added benefit that I have not tested, is simpler code sniffing of this file simply from having less complicated PHP & HTML intermingled.
#4
in reply to:
↑ 3
@
5 years ago
Replying to johnjamesjacoby:
What do you think?
Ah, I have no objections to fixing this jumping in and out of <?php
tags, was just wondering about the consistency of it at least for this file.
Let's go ahead then :)
#6
@
5 years ago
@SergeyBiryukov it felt strange to see it. I wondered if there was a reason for it and then realised it is there purely because if indenting in the file source. That didn't feel right so I thought I'd open a ticket
@johnjamesjacoby thanks and agreed
#7
follow-up:
↓ 12
@
5 years ago
- Keywords close added; commit removed
We should remove the whitespace because it isn't necessary.
That's not entirely true :)
White space in HTML is ignored. White space in the PHP code is used for formatting and better readability. Looking at the patches here, it seems they are not 100% compatible with the coding standards?
Also, consistency-wise, there are many many similar places in WP, do we convert them all to echo '<string ...' . sprintf( .... ) . 'more strings';
? Don't think that's better readability, and if indeed it's needed, seems it will have to be added to the coding standards (the written standard, perhaps also to WPCS)?
#9
in reply to:
↑ 3
@
5 years ago
Replying to johnjamesjacoby:
The problem I have always seen with them is the markup they produce is oddly difficult to follow when viewing source.
While modern tooling and browser inspectors have helped improve that experience...
Right. There was a big discussion many years ago whether the PHP or the HTML should be "more readable". Since then the general tendency has been to make the PHP more readable as the HTML is usually "seen" in the browser console and white space is ignored.
I personally don't mind revisiting this discussion, but thinking that nothing has changed there. Better readability in PHP is preferable to some white space in HTML as the latter is reformatted by the browser tools when viewed.
#10
follow-up:
↓ 11
@
5 years ago
Looking at the patches here, it seems they are not 100% compatible with the coding standards
Which standard? 😁
Better readability in PHP is preferable to some white space in HTML as the latter is reformatted by the browser tools when viewed.
Define “better readability.” The patches attached to this ticket look better to me, which is why I’m suggesting them. They are more uniform. They match the surrounding code. They are less syntactically complicated. They are more obvious than breaking in and out of PHP a dozen times if the opposite approach were suggested instead.
Your better is worse for me.
I’m recommending we make these changes here without the need to call a formal Slack meeting to re-discuss what a committee-of-everyone else thinks is best everywhere else, because this shouldn’t be a design-by-committee decision; it’s a tiny quality-of-life improvement in a multisite file.
Case in point are the number of times tickets like this get opened through the years. There is clearly an audience of contributors that does not find the current approach to be “better” nor even correct. It is easily corrected with zero fear of breakage or regression.
#11
in reply to:
↑ 10
@
5 years ago
Replying to johnjamesjacoby:
Which standard? 😁
The use of echo '<p>' . function(...)
vs. ?><p><?php
for HTML tags. Generally the former is not preferred (see the examples)?
Define “better readability.”
In this case being able to quickly figure out what is HTML and what is PHP? Of course that depends on the IDE you're using, code highlighting, etc. but generally echo '<this is HTML>';
is worse? :)
I’m recommending we make these changes here without the need to call a formal Slack meeting...
Hehe, that wasn't my intention.
Case in point are the number of times tickets like this get opened through the years. There is clearly an audience of contributors that does not find the current approach to be “better” nor even correct.
I've looked at quite a few of these tickets and pretty much all I've seen seem to miss the PHP side of things. They usually only talk about "prettifying" HTML output but don't consider source code readability.
OK, which approach is better? Changing this will add more inconsistently formatted/concatenated code. Should changes be made to try to make the outputted HTML as readable as possible and ignore/diminish readability in PHP? I see at least few hundred places that need such changes.
Or should the practice to patch some places to improve HTML "white space" and other places to improve PHP code (adding HTML white space in the process) continue? :)
#12
in reply to:
↑ 7
;
follow-up:
↓ 13
@
5 years ago
Replying to azaozz:
Looking at the patches here, it seems they are not 100% compatible with the coding standards?
phpcbf
shows zero issues with them :)
I've looked at quite a few of these tickets and pretty much all I've seen seem to miss the PHP side of things. They usually only talk about "prettifying" HTML output but don't consider source code readability.
In my opinion, 49059.2.diff aims to improve the consistency of PHP, not so much the resulting HTML.
Even looking at only <h2>
tags here, echo '<h2>' ... '</h2>'
in signup_another_blog()
seems inconsistent with jumping in and out of <?php
tags in similar instances in the rest of the file. I would think it should be one way or the other.
#13
in reply to:
↑ 12
@
5 years ago
Replying to SergeyBiryukov:
phpcbf
shows zero issues with them :)
Yeah, not sure it can "fix" this type of concatenation.
Even looking at only
<h2>
tags here,echo '<h2>' ... '</h2>'
insignup_another_blog()
seems inconsistent with jumping in and out of<?php
tags in similar instances in the rest of the file. I would think it should be one way or the other.
Perhaps we should be "fixing" that instead of adding more and more echo '<HTML>'
? :)
Or if echo '<HTML>'
is the "better way", lets add it everywhere? Having this inconsistency throughout the source code seems to be the bigger problem here imho :)
#14
@
5 years ago
As a side note: perhaps have a look at the proposed changes to the JS coding standards that (I hope) will also be synced with the PHP coding standards: https://make.wordpress.org/core/2019/12/09/proposed-javascript-coding-standards-revisions-for-prettier-compatibility/.
There concatenation "expands" vertically so things like:
echo '<p>' . sprintf( /* translators: %s: Current user's display name. */ __( 'Welcome back, %s. By filling out the form below, you can <strong>add another site to your account</strong>. There is no limit to the number of sites you can have, so create to your heart’s content, but write responsibly!' ), $current_user->display_name ) . '</p>';
will eventually have to be written most likely like this:
echo ( '<p>' . sprintf( /* translators: %s: Current user's display name. */ __( 'Welcome back, %s. By filling out the form below, you can <strong>add another site to your account</strong>. There is no limit to the number of sites you can have, so create to your heart’s content, but write responsibly!' ), $current_user->display_name ) . '</p>' );
(Which, I agree, is a bit more readable.)
#15
follow-up:
↓ 18
@
5 years ago
I've looked at quite a few of these tickets and pretty much all I've seen seem to miss the PHP side of things. They usually only talk about "prettifying" HTML output but don't consider source code readability.
That isn’t the case with this ticket :)
Conscious use of whitespace in the PHP part of the source file for improved readability is fantastic but this ticket isn’t about that. Instead it refers to the use of whitespace not inside the PHP tags. This results in unintended whitespace output and that is what should be avoided. I think the approach used in the patch is a nice improvement.
#16
@
5 years ago
If we are discussing readability in general, in my opinion having logic and HTML in the same file isn’t nice. Twig helps solve that problem by separating the controller and template. I do understand that isn’t how WordPress does things. Finding a suitable pattern that works for us and then being consistent is the important thing
#17
@
5 years ago
1 thing worth mentioning is network traffic. The whitespace we are referring to travels through the network. Whitespace in PHP doesn't. The impact will be negligible obviously
#18
in reply to:
↑ 15
@
5 years ago
Replying to henry.wright:
I think the approach used in the patch is a nice improvement.
Sorry but I don't think so. This is (mostly) a "coding standards" question, and I agree with @johnjamesjacoby that this ticket is not the place to propose or discuss changes there :)
Switching from stopping and starting the interpreter to echo
just makes this part of the code a bit more inconsistent, and a bit harder to read (IDEs can highlight HTML tags nicely but not when they are outputted as static/hard-coded PHP strings).
The general idea in the WP coding standards is that source code readability is more important than controlling white space in the outputted HTML. This includes formatting the source code in a way that is easy to highlight by various IDEs and "source code editors", and keeping HTML and PHP easily distinguishable when reading the code.
#19
follow-up:
↓ 21
@
5 years ago
The general idea in the WP coding standards is that source code readability is more important than controlling white space in the outputted HTML
To me, we should have both. There will be a pattern or standard we can arrive at which does not output unintended whitespace and is readable when viewing the file source
#20
follow-up:
↓ 22
@
5 years ago
just makes this part of the code a bit more inconsistent, and a bit harder to read
Statements like this are not helpful. They minimize the experiences of other contributors who are attempting to improve & simplify code that (we all agree) is not good in its current iteration. "Better" and "worse" are opinions. "Inconsistent" is relative, because this patch is literally only about providing consistency to this file.
@azaozz, since you have a strong opposing opinion, can you please patch this file yourself here so others can better understand what you mean? If you won't, I think @SergeyBiryukov's patch provides much needed consistency to this file, right or wrong by any current or future coding standards, and recommend it be committed as is.
#21
in reply to:
↑ 19
@
5 years ago
Replying to henry.wright:
Yes, that would be ideal :)
And yes, perhaps it's time to look again at the PHP coding standards and try to come up with some changes that will make this better. Don't think implementing a templating system is possible at this point, that ship has sailed long ago. But perhaps can start by looking at the code and come up with an uniform way of "separating" PHP and HTML. Looks like there are currently over 8,000 ?>
and about 700 echo 'string';
in WP. That's a lot of places that need considering :)
#22
in reply to:
↑ 20
@
5 years ago
Replying to johnjamesjacoby:
just makes this part of the code a bit more inconsistent, and a bit harder to read
Statements like this are not helpful.
I'm sorry but I don't see what's "not helpful" here? :)
As posted abode, IDEs can highlight HTML tags nicely but not when they are outputted as static/hard-coded PHP strings.
"Better" and "worse" are opinions. "Inconsistent" is relative, because this patch is literally only about providing consistency to this file.
...
can you please patch this file yourself here so others can better understand what you mean?
Yep, I understand. My opinion is that "white space" changes like this one should not be done one-file-at-a-time and should be considered as part of the coding standards. Otherwise this results in adding more and more "cases" to the 8,000 ?>
and about 700 echo 'string';
which doesn't "fix" anything. That's why I added the close
keyword.
#23
follow-up:
↓ 24
@
5 years ago
- Owner johnjamesjacoby deleted
I'm sorry but I don't see what's "not helpful" here? :)
Please read the sentence immediately following the one you quoted.
As posted abode, IDEs can highlight HTML tags nicely but not when they are outputted as static/hard-coded PHP strings.
This has nothing to do with the issue being discussed.
My opinion is that "white space" changes like this one should not be done one-file-at-a-time
My opinion (and 15 years of WordPress history supports this) is if improvements like these do not happen a little bit at time, they will never get done at all.
It is completely nonsensical (and against your own points) to argue for changing 8000 lines at once, many of which have nothing to do with the simple issue being discussed here.
That's why I added the close keyword.
I added the commit keyword originally, and you removed it. Keyword wars are uncooperative. Arguments like this one are wholly unproductive. The notifications from this ticket are causing me too much anxiety, because you're wrong and you're not listening and you're not trying to be helpful, so I'm unsubscribing.
#24
in reply to:
↑ 23
@
5 years ago
Replying to johnjamesjacoby:
I added the commit keyword originally, and you removed it. Keyword wars are uncooperative.
This is not a "keyword war" :) In my opinion this is not a good change to make. Removing the "commit" keyword is the result of that review. I'm sorry if I'm failing to express that more convincingly/in better words.
Over the last couple of days I've been trying to convince you and the original poster that even small changes like this one have to be assessed from all possible sides. The proposed change is a small enhancement to the outputted HTML white space and at the same time a small decrease to source code readability mostly because it removes code highlighting.
Arguments like this one are wholly unproductive. The notifications from this ticket are causing me too much anxiety, because you're wrong and you're not listening and you're not trying to be helpful, so I'm unsubscribing.
Sorry if you feel that way, and yes, this is causing me a lot of anxiety too. All I tried to do was a simple review of a small code change that seems to slightly enhance one thing but slightly diminish another.
#25
follow-up:
↓ 26
@
5 years ago
This is not a "keyword war" :)
You removed mine and added yours. You disagree with my opinion and I disagree with yours. Would you like it if I removed your keyword and put mine back? Of course not.
Over the last couple of days I've been trying to convince you and the original poster that even small changes like this one have to be assessed from all possible sides.
We assessed all of those sides ourselves, and came to a different conclusion.
The proposed change is a small enhancement to the outputted HTML white space and at the same time a small decrease to source code readability mostly because it removes code highlighting.
These are your opinions and are not representative of the coding standards or the opinions of others in this ticket. And the syntax highlighting is abysmal with no change due to the lack of surrounding whitespace.
Sorry if you feel that way, and yes, this is causing me a lot of anxiety too. All I tried to do was a simple review of a small code change that seems to slightly enhance one thing but slightly diminish another.
We all have good intentions, and we all are trying our best, even when we do not agree. I choose to believe that’s a baseline understanding, particularly for all of us in this conversation after all of our lovely years together. 🤝
I’d like to see some action taken in this multisite file to resolve this issue. We have a proposed patch on the floor that a multisite component maintainer would like to commit. @azaozz how would you like to proceed?
#26
in reply to:
↑ 25
@
5 years ago
- Keywords close removed
Replying to johnjamesjacoby:
You removed mine and added yours. You disagree with my opinion and I disagree with yours. Would you like it if I removed your keyword and put mine back? Of course not.
Uhhh, sorry I couldn't explain this better. There's no "yours" and "mine" keywords. I tried to review the changes in the patch and they looked "nice to have". But then while looking at the code noticed there were some "side effects" that I thought would need additional consideration. So I removed the "commit" keyword in order to allow for that consideration, nothing more :)
We assessed all of those sides ourselves, and came to a different conclusion.
Great. If you think the enhancement here is more important than the minor "loss" of source code highlighting, go ahead and commit this change. I don't mind, as long as we are "on the same page" and the change has been considered from all possible sides.
We all have good intentions, and we all are trying our best, even when we do not agree. I choose to believe that’s a baseline understanding, particularly for all of us in this conversation after all of our lovely years together. 🤝
Exactly, very well put :)
These are your opinions and are not representative of the coding standards or the opinions of others in this ticket.
Right. While looking at the patch I also looked at all of the WP source code and there are quite a few cases where this is inconsistent. Also looked at the coding standards and they seem a bit unclear. Best course of action (for the future) would be to clarify the coding standards and then apply them across all of the code base. Of course this cannot be done from this ticket, it's just a "side note" here.
(Removing the "close" keyword as it will probably be a while before the coding standards can be clarified and "uniform" changes applied everywhere.)
#29
follow-up:
↓ 33
@
5 years ago
I don't really want to extend the scope of this ticket but should we take the opportunity to escape the translated strings?
This ticket was mentioned in Slack in #core by david.baumwald. View the logs.
5 years ago
#31
@
5 years ago
- Keywords dev-feedback added
Based on the question of increasing the scope, marking for dev-feedback
. @SergeyBiryukov Thoughts?
#32
@
5 years ago
- Milestone changed from 5.4 to Future Release
This ticket still needs a decision, and with 5.4 Beta 1 landing today, this is being moved to Future Release
. If any maintainer or committer feels this can be included in 5.4 or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.
#33
in reply to:
↑ 29
@
5 years ago
Replying to henry.wright:
I don't really want to extend the scope of this ticket but should we take the opportunity to escape the translated strings?
Core translations (including bundled themes) are considered safe because we have a review process for them, see #42639 and the discussion in #30724. (Also related: #32233.)
In WordPress core and bundled themes, strings are generally only escaped in attributes or in <option>
tags.
Thanks @henrywright. Will get this cleaned up imminently.