? ? Pending

User tests: Successful: Unsuccessful:

avatar zero-24
zero-24
15 May 2020

Pull Request for Issue #29108

Summary of Changes

This here just backports the new CssIdentifierRule to 3.10 so extensions can use the new validation rule in 3.10 too.

Testing Instructions

code review

Expected result

Extension devs can use the new validation rule in 3.10 and 4.0

Documentation Changes Required

This behavior will be documented together with the 4.0 PR.

avatar zero-24 zero-24 - open - 15 May 2020
avatar zero-24 zero-24 - change - 15 May 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 15 May 2020
Category Libraries
avatar zero-24 zero-24 - change - 15 May 2020
Labels Added: ? ?
avatar ChristineWk ChristineWk - test_item - 15 May 2020 - Tested successfully
avatar ChristineWk
ChristineWk - comment - 15 May 2020

I have tested this item successfully on 497db9f


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/29109.

avatar mbabker
mbabker - comment - 15 May 2020

This is B/C breaking in the context of 3.x and how the module suffix class is actually used.

avatar HLeithner
HLeithner - comment - 15 May 2020

This is B/C breaking in the context of 3.x and how the module suffix class is actually used.

Why is this a B/C break? he only adds a class that's not used by the core in 3.10 and is intended for 3rd party developers to prepare there extension.

avatar mbabker
mbabker - comment - 15 May 2020

Anyone who uses it cannot use it in conjunction with the module class suffix as is used in 3.x, or other class suffix structures where there is no arbitrary leading space. The rule strips out a number of characters that would be used to indicate a modifier to the class instead of appending (i.e. with <div class="module<?php echo $suffix; ?>"> I cannot have $suffix = '--large' as the rule indicates this BEM pattern is not allowed as the first characters, similar for $suffix = '-100' or $suffix = '100').

This would be better left in 4.0 only instead of adding a class that adds some pretty hefty restrictions that would only cause end user confusion (why can I put this on X module but not on Y).

avatar zero-24
zero-24 - comment - 15 May 2020

The rule strips out a number of characters that would be used to indicate a modifier to the class instead of appending (i.e. with

I cannot have $suffix = '--large' as the rule indicates this BEM pattern is not allowed as the first characters, similar for $suffix = '-100' or $suffix = '100').

Just to be clear that rule is not there in stone ;) So when we have the requirement to allow --large this can be added.

Do you have some more usecases we should take a look into?

avatar conconnl
conconnl - comment - 15 May 2020

Security or not, the remark in the code clearly states it will break existing sites no matter the UseCase.

Remark: “ // Identifiers cannot start with a digit, two hyphens, or a hyphen followed by a digit.”

Any site which upgrades to 3.10 and any change done by a user in a location where a class is used, which get denied by this check will break stuff.

This is not only irritating for the users, also for any site maintainer, developer whatsoever.
Not to mention the negative marketing it will generate with a possible (most likely) B/C break like this.

I’m not planning to fix sites where by any chance the class starts with -Digit, — or just digit.

avatar SharkyKZ
SharkyKZ - comment - 15 May 2020

@conconnl this introduces the validation rule but does not apply it to any existing fields in core. So it's completely optional in 3.10. The PR for 4.0 does apply the rule to all class fields.

But I agree, the regex needs to be changed to allow existing suffix uses. This applies to 4.0 as well. Even though core layouts no longer allows class suffixes, nothing should stop users and template developers from fixing this themselves.

avatar zero-24
zero-24 - comment - 15 May 2020

Remark: “ // Identifiers cannot start with a digit, two hyphens, or a hyphen followed by a digit.”

Yes when you check where that code is based on you notice that they use is different that we do in our code.

So please let me know how you use this field and we make sure the validation rule supports that but we have to start somewhere.

This is not only irritating for the users, also for any site maintainer, developer whatsoever.
Not to mention the negative marketing it will generate with a possible (most likely) B/C break like this.

As mention above there is zero impact on 3.10 installations. ;)

avatar zero-24
zero-24 - comment - 15 May 2020

But I agree, the regex needs to be changed to allow existing suffix uses. This applies to 4.0 as well. Even though core layouts no longer allows class suffixes, nothing should stop users and template developers from fixing this themselves.

Please add examples of what you use so we can make sure it passes the validation rule.

avatar zero-24
zero-24 - comment - 23 May 2020

@mbabker @conconnl @mvanvu The latest changes form the 4.x PR has been merged in here too. That should fix the issues mention here. Feel free to let me know anything that is still a problem so we can adress them.

avatar zero-24
zero-24 - comment - 17 Oct 2020

I'm going to close here as between 3.x and 4.x there are fundamentally changes in 4.x there is no sfx any more but in 3.x we usually have a sfx so better not port something back that can not be used (as in 3.x you use sfx) and would be different once updated to 4.x

avatar zero-24 zero-24 - change - 17 Oct 2020
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2020-10-17 17:33:43
Closed_By zero-24
avatar zero-24 zero-24 - close - 17 Oct 2020

Add a Comment

Login with GitHub to post a comment