? Success

User tests: Successful: Unsuccessful:

avatar mbabker
mbabker
10 May 2015

There is a condition where the multilingual status module and patch tester conflict for reasons I've not been able to debug (see joomla-extensions/patchtester#92 for more info), but the solution for this issue seems to be in the status module.

JHtmlBootstrap::modal is documented as broken since 3.4.0 yet is still used in the module. Removing the call to the broken method seems to fix the conflict I'm hitting without breaking the module.

Testing Instructions

  1. Install the latest com_patchtester build
  2. Enable the Multilanguage status module for the admin side
  3. With your browser's debug console enabled, navigate to the patch tester
  4. Notice there's a Uncaught TypeError: Cannot read property 'options' of null message The component's "Fetch Data" modal nor the modal for the module will work correctly either.
  5. Apply the patch and repeat, there should be no JavaScript issues and both modals should work correctly.

Added Bonus

Codestyle fixes in the module's layout and correctly calling the JHtmlBootstrap::renderModal() method through the JHtml::_ loader.

avatar mbabker mbabker - open - 10 May 2015
avatar dgt41
dgt41 - comment - 10 May 2015

@test ok!
do we need to call jquery.framework here?
Both isis and hathor load jquery and bootstrap...

avatar mbabker
mbabker - comment - 10 May 2015

Yes it should be called. An extension, core or otherwise, should not depend on something else loading its dependencies.

avatar dgt41
dgt41 - comment - 10 May 2015

So maybe we should as well add JHtml::_('bootstrap.framework'); since we also depend on that!

avatar dgt41
dgt41 - comment - 10 May 2015

Never mind that I see that BS loads itself!

avatar mbabker
mbabker - comment - 10 May 2015

I'd say it's a separate PR, I'd also say you could actually totally remove that call as renderModal calls bootstrap.framework which calls jquery.framework.

avatar dgt41
dgt41 - comment - 10 May 2015

JHtmlBootstrap::renderModal( vs JHtml::_('bootstrap.renderModal'
The first one on my IDE with cmd + click on JHtmlBootstrap takes me to the correct file/class but the other takes me to JHtml and cmd + click on 'bootstrap.renderModal' isn’t really helpful as well. So I am a little bit confused here...

avatar mbabker
mbabker - comment - 10 May 2015

JHtml has some code that can let you override the various methods if called via JHtml::_. Direct calling of JHtml (and its subclasses) methods break that functionality. The downside is just as you pointed out, IDE autocompletion is out the window.

avatar smz
smz - comment - 10 May 2015

IMHO JHtmlBootstrap::Modal() should be patched and just return;: its code is broken and useless.

For the reasons why it is still there, please see: #5087 (comment)

avatar mbabker
mbabker - comment - 10 May 2015

If someone can fix the broken method then I'd say do that, otherwise you're right that it should just return if it's fatally broken.

Still a separate PR. We have a bad habit of breaking single responsibility principles in our code and our patches (and I do realize I'm now the pot calling the kettle black considering what I changed in the layout file here).

avatar smz
smz - comment - 10 May 2015

AFAIK, it was meant as a support method for the "old" JHtmlBootstrap::renderModal() but it is unneed in that role right now.

The rationale for keeping it as it was is that somewone could have used it for other reasons and removing or modifying it could have been a breach of B/C. Personally I don't worship to the B/C god that far, but I think that removing it is a decision that should be taken by the PLT.

avatar mbabker
mbabker - comment - 10 May 2015

If it's fatally broken then I'd say dump the code and just make it return, leave a comment explaining why. As for why you can't just remove the method completely, it's a publicly callable API method. Leaving it doing nothing doesn't break anyone's code (unless they depend on that broken JS) but removing it entirely results in fatal errors for not found methods. There's no pleasing everyone here, but if we know the method's broken then do something about it other than deprecate it with a vague comment.

avatar smz
smz - comment - 10 May 2015

I totally agree... :+1:
Do you want me to create a PR along such lines?

avatar mbabker
mbabker - comment - 10 May 2015

I hope someone does. I need to put GitHub on ignore and get back to writing talks :wink:

avatar zero-24 zero-24 - change - 10 May 2015
Category Administration Modules
avatar zero-24 zero-24 - change - 10 May 2015
Status New Ready to Commit
Easy No Yes
avatar zero-24
zero-24 - comment - 10 May 2015

@test successful. Both patchtester and the modul still works. Thanks :smile: RTC


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

avatar zero-24 zero-24 - change - 10 May 2015
Labels Added: ? ?
avatar zero-24 zero-24 - change - 10 May 2015
Labels Added: ?
avatar zero-24 zero-24 - change - 10 May 2015
Labels Added: ?
avatar zero-24 zero-24 - test_item - 10 May 2015 - Tested successfully
avatar zero-24 zero-24 - alter_testresult - 10 May 2015 - dgt41: Tested successfully
avatar smz
smz - comment - 10 May 2015

@mbabker: please see #6919

avatar infograf768
infograf768 - comment - 11 May 2015

@test successful for multilanguage status module

avatar zero-24 zero-24 - alter_testresult - 11 May 2015 - infograf768: Tested successfully
avatar zero-24 zero-24 - close - 11 May 2015
avatar mbabker mbabker - change - 11 May 2015
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2015-05-11 08:20:04
Closed_By mbabker
avatar mbabker mbabker - close - 11 May 2015
avatar mbabker mbabker - close - 11 May 2015
avatar zero-24 zero-24 - change - 11 May 2015
Milestone Added:
avatar zero-24 zero-24 - change - 14 Oct 2015
Labels Removed: ?

Add a Comment

Login with GitHub to post a comment