Remove bootstrap.modal call in multilang status module#6918
Remove bootstrap.modal call in multilang status module#6918mbabker wants to merge 1 commit intojoomla:stagingfrom mbabker:multilangModal
Conversation
|
@test ok! |
|
Yes it should be called. An extension, core or otherwise, should not depend on something else loading its dependencies. |
|
So maybe we should as well add |
|
Never mind that I see that BS loads itself! |
|
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. |
|
|
|
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. |
|
IMHO For the reasons why it is still there, please see: #5087 (comment) |
|
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). |
|
AFAIK, it was meant as a support method for the "old" 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. |
|
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. |
|
I totally agree... 👍 |
|
I hope someone does. I need to put GitHub on ignore and get back to writing talks 😉 |
|
@test successful. Both patchtester and the modul still works. Thanks 😄 RTC This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6918. |
|
@test successful for multilanguage status module |
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
Uncaught TypeError: Cannot read property 'options' of nullmessage The component's "Fetch Data" modal nor the modal for the module will work correctly either.Added Bonus
Codestyle fixes in the module's layout and correctly calling the JHtmlBootstrap::renderModal() method through the JHtml::_ loader.