Make WordPress Core

Opened 20 months ago

Closed 20 months ago

Last modified 20 months ago

#57002 closed defect (bug) (wontfix)

safecss_filter_attr() removes the trailing semincolon

Reported by: chouby's profile Chouby Owned by:
Milestone: Priority: normal
Severity: trivial Version: 2.8.1
Component: Formatting Keywords: has-patch has-unit-tests
Focuses: css Cc:

Description

A trailing semicolon is not mandatory at the end of CSS rules. It's however a best practice to add one.

However the output of safecss_filter_attr( 'margin-right: 20px;' ) is margin-right: 20px. I would expect that it keeps margin-right: 20px

Change History (7)

#1 @Chouby
20 months ago

Copy paste drama :/. Of course, one should read:

I would expect that it keeps margin-right: 20px;

This ticket was mentioned in PR #3579 on WordPress/wordpress-develop by @hugod.


20 months ago
#2

  • Keywords has-patch has-unit-tests added

Ticket https://core.trac.wordpress.org/ticket/57002

## Description

  • Add tests for safecss_filter_attr().
  • Add a ; before returning filtered CSS in safecss_filter_attr().

@hugod commented on PR #3579:


20 months ago
#3

I'm aware some tests are failing. This is due to the ; added in any cases...
I'll fix them once the ticket is validated.

#4 @peterwilsoncc
20 months ago

Thanks for the ticket.

I agree that it would be nicer if safecss_filter_attr() retained the trailing semi-colon but I worry that it's a little late to change the function.

If a plugin has the following code then the change would cause the style attribute to contain the string ;;.


<div
  style="<?php echo safecss_filter_attr( wp_unslash( $_GET[ 'user_css' ] ) ); ?>; more:css;" />
</div>

It's something browsers' error recovery should be able to handle but, as with the lack of trailing semi-colon, it's not very nice code.

Last edited 20 months ago by peterwilsoncc (previous) (diff)

#5 @hugod
20 months ago

Hi @peterwilsoncc
Thanks for the feedback, I didn't think about it that way.
Should I close my PR?

#6 @peterwilsoncc
20 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

@hugod Thanks for understanding, yes, let's close this off without a fix for now

To be clear, I absolutely agree with the premise that a trailing semi-colon would be best practice when style="safecss_filter_attr( /*styles */ )"; the risk of ending up with two consecutive semi-colons results in a relative decision about the nicer code.

Some choices require choosing the pragmatic over the ideal.

Note: See TracTickets for help on using tickets.