Joomla! Issue Tracker | Joomla! CMS #32357 - [4.0] Mitigate some b/c breaks2

NPM Resource Changed ? ?

Fixed in Code Base
12 Feb 2021
Medium
Build: 4.0-dev
# 32357
Diff
dgrammatiko:4.0-dev_bc_breaks2

Pending

User tests: Successful: Unsuccessful:

dgrammatiko
8 Feb 2021

Another take of #32146 .

Summary of Changes

Keep the functions endTab, addTab, endTabSet, startTabSet, renderModal, endSlide, addSlide, endAccordion, startAccordion with the same selector as Joomla 3. No B/C break here!

For the functions toast, tooltip, tab, scrollspy, popover, modal, dropdown, collapse, carousel, button, alert there is no default CSS Selector and also the methods can accept any valid CSS Selector. How this works? Here's an overview:

If a function is called without a selector Joomla will include the appropriate script BUT WILL NOT TRY TO INITIALISE ANYTHING (useful if you want to do something programmatically on the client side).
If a function is called with a selector and no options Joomla will include the appropriate script AND WILL TRY TO INITIALISE the given selector (if it exists in the document) with the Bootstrap Defaults for the component
If a function is called with a selector and with provided options Joomla will include the appropriate script AND WILL TRY TO INITIALISE the given selector (if it exists in the document) with the GIVEN options for the component

Testing Instructions

Try to select another component in a menu administrator/index.php?option=com_menus&view=item&client_id=0&layout=edit&id=101 Accordion still works
Try to edit the tinyMCE options, tabs for the sets still work
Edit an article, Modals for user and images still work
Dropdown like save as (toolbar) and user menu (top bar) still work

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Documentation Changes Required

edf8282 8 Feb 2021

Another take

dgrammatiko - open - 8 Feb 2021

dgrammatiko - change - 8 Feb 2021

Status

New

⇒

Pending

joomla-cms-bot - change - 8 Feb 2021

Category

Administration Layout Libraries Modules Front End

⇒

Administration Front End com_users Layout Libraries Modules

0c294ee 8 Feb 2021

Update libraries/src/HTML/Helpers/Bootstrap.php

536fcc9 8 Feb 2021

Update Bootstrap.php

richard67 - comment - 10 Feb 2021

@dgrammatiko Are those preg_replace still necessary, and are they right?

dgrammatiko - comment - 10 Feb 2021

Are those preg_replace still necessary, and are they right?

These are fail safe fuses. Lets take public static function addTab($selector, $id, $title) :string as an example:
in J3 it expects an ID (without . or # )

joomla-cms/libraries/cms/html/bootstrap.php

Lines 790 to 805 in e6cbda0

    
           public static function addTab($selector, $id, $title) 
        
           { 
        
           	static $tabScriptLayout = null; 
        
           	static $tabLayout = null; 
        
           	$tabScriptLayout = $tabScriptLayout === null ? new JLayoutFile('libraries.cms.html.bootstrap.addtabscript') : $tabScriptLayout; 
        
           	$tabLayout = $tabLayout === null ? new JLayoutFile('libraries.cms.html.bootstrap.addtab') : $tabLayout; 
        
           	$active = (static::$loaded['JHtmlBootstrap::startTabSet'][$selector]['active'] == $id) ? ' active' : ''; 
        
           	// Inject tab into UL 
        
           	JFactory::getDocument() 
        
           		->addScriptDeclaration($tabScriptLayout->render(array('selector' => $selector, 'id' => $id, 'active' => $active, 'title' => $title))); 
        
           	return $tabLayout->render(array('id' => $id, 'active' => $active)); 
        
           }

and then passes the selector to the layout https://github.com/joomla/joomla-cms/blob/staging/layouts/libraries/cms/html/bootstrap/addtab.php but if you pass something with . or # will break badly

In J4 we make sure that . or # will not be passed as a selector to the layout (it will break things if we don't check) https://github.com/joomla/joomla-cms/pull/32357/files#diff-e89418d6aa7e99964d6509d6b9ccb765ec5b9edd3482bd8ad08c29f29d2202e5R817 and https://github.com/joomla/joomla-cms/pull/32357/files#diff-74bf0060b6aa5eed515b6e29b0b4242ab49049d00b711b755f4c96488407fd53R16

Maybe overprotective but better safe than sorry

richard67 - comment - 10 Feb 2021

@dgrammatiko But why is that ? modifier in the pattern only applied to the \. but not the #? Or isn't that a modifier but a literal ??

dgrammatiko - comment - 10 Feb 2021

@richard67 unless I got it totally wrong /^\.?#/ should be returning either . or # at the beginning of the string. I think I just copied that from some method in the J3 repo

richard67 - comment - 10 Feb 2021

@richard67 unless I got it totally wrong /^\.?#/ should be returning either . or # at the beginning of the string. I think I just copied that from some method in the J3 repo

So a . or # at the beginning shall be removed? If so, then your pattern seems not to do that, at least not when I've tested with diverse online tools. A # was removed, but a . not. If it shall be that one . or one # at the beginning (and only there) shall be removed, then I would think pattern /^[\.#]/ is the right one.

67e7c07 10 Feb 2021

Change the regex

dgrammatiko - comment - 10 Feb 2021

@richard67 fair enough, I'm not a regex expert but I'll take your word on this one. Changes done in 67e7c07

richard67 - comment - 10 Feb 2021

@richard67 fair enough, I'm not a regex expert but I'll take your word on this one. Changes done in 67e7c07

Then I only can hope I was not wrong ;-)

laoneo - comment - 10 Feb 2021

Menu items do work, but the code <?php echo JHtml::_('bootstrap.startTabSet', 'com-dpcalendar-form', ['active' => 'general']); ?> and <?php echo JHtml::_('bootstrap.addTab', 'com-dpcalendar-form', 'general, 'The label'); ?>does not work.Getting:

f33d2ac 10 Feb 2021

Fix tabs

joomla-cms-bot - change - 10 Feb 2021

Category

Administration Layout Libraries Modules Front End com_users

⇒

Administration JavaScript Repository NPM Change Front End com_users Layout Libraries Modules

dgrammatiko - comment - 10 Feb 2021

@laoneo should be ok after f33d2ac

6361b58 10 Feb 2021

oops

dgrammatiko - change - 10 Feb 2021

Labels

Added: NPM Resource Changed

laoneo - comment - 10 Feb 2021

Getting now

laoneo - comment - 10 Feb 2021

You can test it with DPCalendar from here https://joomla.digital-peak.com/download/dpcalendar/dpcalendar-8-0-3?format=html

8f10433 10 Feb 2021

oops

dgrammatiko - comment - 10 Feb 2021

@laoneo hopefully now it's fine

laoneo - test_item - 11 Feb 2021 - Tested successfully

laoneo - comment - 11 Feb 2021

I have tested this item ✅ successfully on 8f10433

Without the patch the following system tests have been broken:

Selecting a menu item
Tabs in back end forms
User select field in back end forms

All of them do work now with the pr.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/32357.}

chmst - test_item - 11 Feb 2021 - Tested successfully

chmst - comment - 11 Feb 2021

I have tested this item ✅ successfully on 8f10433

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/32357.}

richard67 - change - 11 Feb 2021

Status

Pending

⇒

Ready to Commit

richard67 - comment - 11 Feb 2021

RTC

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/32357.}

infograf768 - comment - 11 Feb 2021

See #32259 (comment)

richard67 - comment - 11 Feb 2021

@infograf768 So remove RTC?

dgrammatiko - comment - 11 Feb 2021

@infograf768 #32259 is something specific to the media field, nothing touched here could solve that problem

richard67 - comment - 11 Feb 2021

@infograf768 #32259 is something specific to the media field, nothing touched here could solve that problem

@dgrammatiko Do you think you can fix that with another PR?

dgrammatiko - comment - 11 Feb 2021

Do you think you can fix that with another PR?

It should be a simple conditional for the user access level (if not allowed to edit just display the image or don't display anything at all) it's not even js related.

infograf768 - comment - 11 Feb 2021

I agree it's not related to this patch. Was just responding to #32259 (comment)

dgrammatiko - comment - 11 Feb 2021

Posting something like See #32259 (comment) without context is very confusing, as both I and @richard67 thought that this was an unwanted side effect

richard67 - comment - 11 Feb 2021

No, all fine, it's only me being paranoid sometimes when it's related to software ... I consider everything possible.

wilsonge - comment - 11 Feb 2021

@dgrammatiko fix the conflicts from the BS beta 2 merge - one test to ensure that new beta doesn't change anything and good to merge

6f50ef3 11 Feb 2021

Merge branch '4.0-dev' of github.com:joomla/joomla-cms into 4.0-dev_bc_breaks2

dgrammatiko - change - 11 Feb 2021

Labels

Added: ?

richard67 - comment - 12 Feb 2021

@laoneo @chmst Could one of you give it a quick test if it still works after Dimitris has fixed the conflicts so the PR uses BS5 Beta 2 now? Thanks in advance.

laoneo - comment - 12 Feb 2021

A first test run failed because the change event is not triggered in the user field. But can also be an issue in our end. Will test over weekend.

dgrammatiko - comment - 12 Feb 2021

@laoneo that sounds like a side effect from #32379

There's a PR for this issue: #32397

chmst - comment - 12 Feb 2021

Tested again successfully together with #32397.

laoneo - test_item - 12 Feb 2021 - Tested successfully

laoneo - comment - 12 Feb 2021

I have tested this item ✅ successfully on 6f50ef3

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/32357.}

laoneo - comment - 12 Feb 2021

So we have two successful tests here.

wilsonge - close - 12 Feb 2021

wilsonge - merge - 12 Feb 2021

wilsonge - change - 12 Feb 2021

Status	Ready to Commit	⇒	Fixed in Code Base
Closed_Date	0000-00-00 00:00:00	⇒	2021-02-12 16:06:17
Closed_By		⇒	wilsonge

wilsonge - comment - 12 Feb 2021

Thanks guys ?!

Add a Comment

Older
Newer

Joomla! Issue Tracker - CMS

[#32357] - [4.0] Mitigate some b/c breaks2

Summary of Changes

Testing Instructions

Actual result BEFORE applying this Pull Request

Expected result AFTER applying this Pull Request

Documentation Changes Required

Add a Comment

	public static function addTab($selector, $id, $title)
	{
	static $tabScriptLayout = null;
	static $tabLayout = null;

	$tabScriptLayout = $tabScriptLayout === null ? new JLayoutFile('libraries.cms.html.bootstrap.addtabscript') : $tabScriptLayout;
	$tabLayout = $tabLayout === null ? new JLayoutFile('libraries.cms.html.bootstrap.addtab') : $tabLayout;

	$active = (static::$loaded['JHtmlBootstrap::startTabSet'][$selector]['active'] == $id) ? ' active' : '';

	// Inject tab into UL
	JFactory::getDocument()
	->addScriptDeclaration($tabScriptLayout->render(array('selector' => $selector, 'id' => $id, 'active' => $active, 'title' => $title)));

	return $tabLayout->render(array('id' => $id, 'active' => $active));
	}