Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#42578 closed defect (bug) (fixed)

PHP functions inside <p> tags creates new <p> tag, breaking the parent tag into two.

Reported by: karthikeyankc's profile karthikeyankc Owned by: dd32's profile dd32
Milestone: 4.9.2 Priority: normal
Severity: normal Version: 4.9
Component: Formatting Keywords: revert good-first-bug has-patch fixed-major
Focuses: template Cc:

Description

Expected behaviour:

<p class="bio-desc"><?php the_author_meta('description'); ?></p>

should render as:

<p class="bio-desc">Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat. </p>

What happens:

<p class="bio-desc"></p><p>Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat.</p>
<p></p>

Attachments (4)

42578.diff (2.6 KB) - added by 0x6f0 7 years ago.
42578.2.diff (663 bytes) - added by 0x6f0 7 years ago.
42578.3.diff (3.2 KB) - added by peterwilsoncc 7 years ago.
42578.4.diff (2.4 KB) - added by peterwilsoncc 7 years ago.

Download all attachments as: .zip

Change History (47)

#1 follow-up: @swissspidy
7 years ago

  • Keywords close added

It's a feature, not a bug. See #40040.

#2 in reply to: ↑ 1 @plexusllc
7 years ago

Replying to swissspidy:

It's a feature, not a bug. See #40040.

It's a bug not a feature. Wrong, Bad, and Dumb to commit changes to core that break themes everywhere without a word. #40040 was a bad idea.

#3 @obenland
7 years ago

  • Keywords close removed
  • Milestone changed from Awaiting Review to 4.9.2

I think we should consider reverting this. It doesn't seem like it's forward compatible, as hinted by failing tests after it was introduced. It creates a visual regression that is hard to mitigate without updating every theme that displays author meta, following the example set by default themes.

@johnbillion, what are your thoughts?

#4 @obenland
7 years ago

It looks like it breaks themes in two ways:

Some themes escape the author bio output: https://kathrynwp-automated-transfer-001.blog/2013/09/03/post-1/

As mentioned in the ticket description, some themes wrap the author bio in paragraph tags, leading to double margins/paddings.

#5 @johnbillion
7 years ago

  • Focuses template added
  • Keywords needs-patch revert added

Agreed. I didn't consider the consequences of this enough.

#6 @henry.wright
7 years ago

The aim of #40040 was to make output consistent by running wptexturize, convert_chars, wpautop and shortcode_unautop on the author description. This is what's done to post content, category descriptions etc.

#40040 hasn't been popular with some theme authors because the change may result in nested <p> tags if <p> tags are already used in author description markup.

I agree that this needed more testing.

#7 @obenland
7 years ago

  • Keywords good-first-bug added

#8 follow-up: @0x6f0
7 years ago

Hi, I would like to try this one as my first patch. As I understand this was to just revert 40040?

I reverted the changes and it seems to be working as expected now. If I'm missing something please let me know.

Thank you

@0x6f0
7 years ago

#9 in reply to: ↑ 8 @Ov3rfly
7 years ago

Replying to 0x6f0:

If I'm missing something please let me know.

The reason for this ticket is missing: The 'get_the_author_description' in filter array.

To fix this ticket, it is actually enough to just remove 'get_the_author_description', no other changes necessary.

https://i.imgur.com/BXykRY7.png

This ticket was mentioned in Slack in #forums by zoonini. View the logs.


7 years ago

@0x6f0
7 years ago

#11 @0x6f0
7 years ago

Thank you @Ov3rfly

Sorry, my test case was off. I should've followed exactly what @karthikeyankc had but I changed it to

<p class="bio-desc"><?php the_author_meta('user_description'); ?></p>

which doesn't behave the same. So I thought I had it fixed.

It's working now though, sorry again.

#12 @drywallbmb
7 years ago

  • Keywords has-patch added; needs-patch removed

#13 @drywallbmb
7 years ago

  • Keywords needs-patch added; has-patch removed

This ticket was mentioned in Slack in #themereview by iceable. View the logs.


7 years ago

#15 @williampatton
7 years ago

Are we looking at a likely chance of reverting this change?

Since this was implimented I have been undertaking the grueling tasks of trying to find all affected authors and notify them when there themes usage has been affected. There are several hundred themes affected in the directory including at least 1 core theme.

If this is to be reverted I can stop doing that but if not then I'll need to confine reaching out to all those authors and informing them.

#16 follow-up: @swissspidy
7 years ago

@williampatton yes, this is currently slated for 4.9.2

#17 in reply to: ↑ 16 @williampatton
7 years ago

Replying to swissspidy:

@williampatton yes, this is currently slated for 4.9.2

Awesome, thanks for update :)

I seen the 4.9.2 tagging but know that sometimes tagged items get pushed back, seeing the patch tag was removed got me worried :p

#18 @rabmalin
7 years ago

  • Keywords has-patch added; needs-patch removed

Patch looks good from my side.

This ticket was mentioned in Slack in #forums by zoonini. View the logs.


7 years ago

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


7 years ago

#21 @peterwilsoncc
7 years ago

In 42578.3.diff :

  • partial revert of [41232]: changes to post type description output from get_the_archive_description()
  • full revert of [41173]
  • full revert of [41172]
  • the new function get_the_post_type_description() created in #40040 is retained

#22 follow-up: @jeremyescott
7 years ago

I know the choice seems to have been made, I hope I can influence the thought process a little here.

Right now, we are discussing reverting the changes because, basically, some theme developers are frustrated. And I get that. I make themes.

But, as of right now, a handful of theme authors, myself included have already fixed our sites. Its not hard to change a <p> to a <div>, seriously, and if you revert this, I'm going to have to go out and ask my clients again to pay me to revert my changes. (Or, I'm hoping, maybe it won't be required.)

@swissspidy was right, this is a feature. One that was stupid late to the party. One I feel should be kept.

When I make a theme for a client, my goal is to deliver it and them not require me to be on call to make updates. I have a client that has 400 authors. They saw some examples of author info boxes that were richly detailed and they asked me why they can't even get a line break when desired.

I think more people, in the past and moving forward, are more annoyed by the lack of a feature like basic formatting in a user-changeable box, than the group of theme devs who are frustrated by this.

I fully 100% concede that maybe this change wasn't highlighted enough, or promoted enough, but I don't think we should take a step backwards when we know that the audience we're making WordPress for are publishers, not developers. This feature makes it easier for a publisher to make a change without a developer to help them.

This would be sad change to revert.

Further, Matt's keynote at WCUS gave me the impression that we want more and more of this user-friendly formatting in the future. Also, 4.8's WYSIWYG widgets are another example.

I think the theme devs should pause, appreciate the change, and accept that every now and then an update to core might be good for the future of WP, even if it causes a minor headache today.

My 2¢.

#23 in reply to: ↑ 22 @peterwilsoncc
7 years ago

Replying to jeremyescott:

Right now, we are discussing reverting the changes because, basically, some theme developers are frustrated. And I get that. I make themes.

But, as of right now, a handful of theme authors, myself included have already fixed our sites. Its not hard to change a <p> to a <div>, seriously, and if you revert this, I'm going to have to go out and ask my clients again to pay me to revert my changes. (Or, I'm hoping, maybe it won't be required.)

@swissspidy was right, this is a feature. One that was stupid late to the party. One I feel should be kept.

I agree, reverting will penalise theme authors who have already updated their themes.

Between the choices of retaining or reverting, I think reverting is the lesser evil as more themes are likely to be broken at this time than fixed.

For the themes you have fixed, I suggest adding the filters within functions.php as a way to maintain the 4.9 functionality.

  • the authors archive description can be filtered on get_the_author_description
  • the post type archive description will need to be filtered on get_the_archive_description

#24 @jeremyescott
7 years ago

@peterwilsoncc: To recap, you're saying the lesser of two evils is to break themes again?

I can think of many cases where I wouldn't have been against a revert, like during development, during beta testing, even a week after release. But its been 8 weeks. For every stubborn theme dev who hasn't fixed their theme by now, there are many who have. Because its an easy fix. Either change a <p> to a <div> or change the escape from esc_html to wp_kses or something like it. But now you're gonna punish all the people who made that fix because some people didn't want to.

Its foolish to try and measure those who have fixed it against those who haven't and somehow decide who loses. But I will say, the businesses that make up the ecosystem around WP have already fixed it. The premium theme devs have already fixed it. They were fixing it on day 1, or even finding it during beta. They shouldn't get preferential treatment, but you know who should? Our end-users, the publishers.

The reason a lot of people may not have reported it as a bug during dev and beta is simply that it fits the mission of WordPress: to make publishing easier. That line break... its important to an unskilled publisher who is wondering why they can't make a paragraph in their bio. That line break matters. And you gave it to them. Now you want to take it away.

Reverting will fix an unknown number of broken sites while breaking an unknown number of fixed sites while taking away a feature that democratized publishing.

This is a sticky situation, I get it. But for where we are now, the failure here is no longer the half-baked rollout of a feature, per se, it is the lack of communication that got us into this mess and the heel-dragging that sees it still unresolved 8 weeks later. It is hugely irresponsibly late to revert this.

And at this point, we should be considering the impact on our publishers, who've wanted something like this for a long time, and the impact on the professionals whose careers forced them to fix this ASAP instead of waiting 8 weeks for a revert. If this hadn't been committed or if it wasn't out in the wild for 8 weeks already, I'd agree with excluding or reverting it, but since it was, and since tons of theme devs have already fixed their themes/sites, if it comes down to should we fix what is broken thus breaking what was fixed, or leave what is broken broken, shouldn't the mission of making publishing easier be considered when tipping the scales? I think so.

I'm speaking for hard workers out there who just dealt with this problem and moved on. Don't punish them. And I'm speaking for the publishers who've wanted a line break in their bio for years. Don't take that away from them.

Last edited 7 years ago by jeremyescott (previous) (diff)

#25 @rabmalin
7 years ago

Between the choices of retaining or reverting, I think reverting is the lesser evil as more themes are likely to be broken at this time than fixed.

Agree. Although the changes was good but it was breaking change. It went to core pretty silently without even mentioning the dev notes. Any breaking changes should be pushed to core with larger visibility I believe. Lets revert this in this version. In the next major release we can add it again with proper approach.

#26 @Ov3rfly
7 years ago

In the next major release we can add it again with proper approach.

A proper approach would include the_author_meta( 'description' ) changes in Twenty Sixteen, Twenty Fifteen, Twenty Thirteen, Twenty Twelve and all related child themes and all other themes which are based on these official WordPress themes... which is impossible.

#27 follow-up: @dd32
7 years ago

I have to kind-of agree with not reverting this.

  1. The damage is done, and we didn't fix it within a reasonable timeframe (IMHO 8 weeks is not reasonable) which has caused themes to update for it.
  2. The user expectation of being able to add line breaks, and in the future, rich-text is made harder by reverting
  3. Reverting this in 4.9.x, and adding it back in 5.0~5.5 isn't a viable method forward IMHO, all it does is delay breakages, which have already been broken for weeks.

So while I agree reverting was the best method forward 6-8 weeks ago, I'm not sure it's still the best way forward.

I'd be interested in seeing more data about how many themes are actually broken by this now - have the majority of WordPress.org-hosted themes updated? Did they break in the first place?
Does anyone have a copy of all themes locally they could scan through?

#28 @Ov3rfly
7 years ago

I'd be interested in seeing more data about how many themes are actually broken by this now

Twenty Sixteen: 700.000+
Twenty Fifteen: 400.000+
Twenty Thirteen: 100.000+
Twenty Twelve: 300.000+
Child themes of above: uncountable
Custom themes based on these official WordPress themes: uncountable

Reverting .. and adding it back ..

Revert it, do not add it back.

#29 @jeremyescott
7 years ago

@Ov3rfly: All the core theme installs can be fixed by changing 1 line on four themes. I'll volunteer to do if core team commits to not reverting. It will be easy.

Revert it, do not add it back.

So never give publishers rich text options for their bios? That doesn't fit our mission of democratizing publishing. Even if this is reverted, a future with rich text/wysiwyg for all user input fields should be in our heads. Hence why this will be changed one day.

do not add it back.

Why hold WordPress back forever because of a short-term frustration? Fix the affected themes and move on. Its a worthwhile change for our end-user publishers!

#30 @rabmalin
7 years ago

Here are few themes from the Popular page in the directory where themes are not updated for the fix.

  • Customizr - 100k
  • ColorMag - 90k
  • Hueman - 80k
  • Hestia - 60k
  • Astra - 20k

This is not only about 4 default themes. We should consider thousands of themes in the ecosystem. Back Compat is the motto of WordPress. Or if any breaking changes then developers should be well informed prior pushing to the core.

As of Theme Review Team, we have not asked theme authors to fix the author description as we were hoping this will be reverted. Even recently live themes have this issue.

#31 @Ov3rfly
7 years ago

Fix the affected themes and move on. Its a worthwhile change for our end-user publishers!

It is not possible to fix all affected themes.

This "feature" is clearly "plugin-land", plugins for this feature exist since years, no need to mess up the core.

One more affected example, not a theme, but still:

  • JetPack: 4+ Million

(modules/theme-tools/content-options/author-bio.php)

#32 in reply to: ↑ 27 @dd32
7 years ago

Replying to dd32:

I have to kind-of agree with not reverting this.

This was written without realising that the Bundled themes were also affected by this, which downright sucks :)

It's going to mean that some people who have done work to fix it will have only done it for a short-term relief, but hopefully changing it back doesn't affect them too much.

I'm going to backtrack and agree with reverting, with the intention that this is brought back in 5.0 in a backwards-compatible manner.
That may mean going against #8776 which deprecated multiple functions in preference for the_author_meta( 'description' ) and instead deprecating the_author_meta( 'description' ) in preference for a new similarly named the_new_pretty_author_description() type function which does output <p> tags deliberately from inception, and maybe even adds WYSIWYG editing to that field.

Last edited 7 years ago by dd32 (previous) (diff)

#33 @pbearne
7 years ago

Let's get a new function

#34 @plexusllc
7 years ago

Let's get a new function

Exactly.

Revert this breaking change (and that reverting it has taken this long is the problem with all forms of "decisions by committee"), and then add the ability for publishers and theme builders to override the default behavior by opting in to new behavior which leaves the old default behavior alone if the new one is not intentionally used.

That's what should have been done in the first place. That's what should be done now.

And BTW as a "hard working" developer, I say this as someone who had to modify things on sites I manage when they broke in 4.9 and will have to re-modify again when this poorly thought-out change is finally reverted.

This ticket was mentioned in Slack in #themereview by rabmalin. View the logs.


7 years ago

#36 @peterwilsoncc
7 years ago

  • Keywords needs-refresh added
  • Owner set to peterwilsoncc
  • Status changed from new to accepted

Reviewing blame logs:

  • the post type archive portion of get_the_archive_description() wasn't added until 4.9, [40976], it's always been formatted
  • 42578.3.diff will need a refresh to remove the partial revert of [41232].

#37 @peterwilsoncc
7 years ago

  • Keywords commit added; needs-refresh removed

In 42578.4.diff:

  • full revert of [41173]
  • full revert of [41172]
  • the new function get_the_post_type_description() created in #40040 is retained

#38 @peterwilsoncc
7 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 42441:

Formatting: Do not run formatting and texturization on author bios.

Removes the formatting and texturization added to author bios in #40040 due to back-compatibility concerns.

Reverts [41172], [41173].

Props 0x6f0, rabmalin for review.
Fixes #42578.

#39 @peterwilsoncc
7 years ago

  • Keywords fixed-major added; commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for back-porting.

#40 @peterwilsoncc
7 years ago

  • Owner changed from peterwilsoncc to dd32
  • Status changed from reopened to assigned

#41 @dd32
7 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 42445:

Formatting: Do not run formatting and texturization on author bios.

Removes the formatting and texturization added to author bios in #40040 due to back-compatibility concerns.

Reverts [41172], [41173].

Props 0x6f0, rabmalin for review, peterwilsoncc.
Merges [42441] to the 4.9 branch.
Fixes #42578.

This ticket was mentioned in Slack in #core by jeremyescott. View the logs.


7 years ago

This ticket was mentioned in Slack in #core by jeremyescott. View the logs.


7 years ago

Note: See TracTickets for help on using tickets.