Skip to content

Remove bootstrap.modal call in multilang status module#6918

Closed
mbabker wants to merge 1 commit intojoomla:stagingfrom
mbabker:multilangModal
Closed

Remove bootstrap.modal call in multilang status module#6918
mbabker wants to merge 1 commit intojoomla:stagingfrom
mbabker:multilangModal

Conversation

@mbabker
Copy link
Copy Markdown
Contributor

@mbabker mbabker commented May 10, 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.

@dgrammatiko
Copy link
Copy Markdown
Contributor

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

@mbabker
Copy link
Copy Markdown
Contributor Author

mbabker commented May 10, 2015

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

@dgrammatiko
Copy link
Copy Markdown
Contributor

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

@dgrammatiko
Copy link
Copy Markdown
Contributor

Never mind that I see that BS loads itself!

@mbabker
Copy link
Copy Markdown
Contributor Author

mbabker commented May 10, 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.

@dgrammatiko
Copy link
Copy Markdown
Contributor

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...

@mbabker
Copy link
Copy Markdown
Contributor Author

mbabker commented May 10, 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.

@smz
Copy link
Copy Markdown
Contributor

smz commented May 10, 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)

@mbabker
Copy link
Copy Markdown
Contributor Author

mbabker commented May 10, 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).

@smz
Copy link
Copy Markdown
Contributor

smz commented May 10, 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.

@mbabker
Copy link
Copy Markdown
Contributor Author

mbabker commented May 10, 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.

@smz
Copy link
Copy Markdown
Contributor

smz commented May 10, 2015

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

@mbabker
Copy link
Copy Markdown
Contributor Author

mbabker commented May 10, 2015

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

@zero-24
Copy link
Copy Markdown
Contributor

zero-24 commented May 10, 2015

@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.

@zero-24 zero-24 added RTC This Pull Request is Ready To Commit PR-staging labels May 10, 2015
@smz
Copy link
Copy Markdown
Contributor

smz commented May 10, 2015

@mbabker: please see #6919

@infograf768
Copy link
Copy Markdown
Member

@test successful for multilanguage status module

@zero-24 zero-24 added this to the Joomla! 3.4.2 milestone May 11, 2015
photodude pushed a commit to photodude/joomla-cms that referenced this pull request May 15, 2015
johanjanssens pushed a commit to joomlatools/joomla-fork that referenced this pull request Jun 19, 2015
@zero-24 zero-24 removed the RTC This Pull Request is Ready To Commit label Oct 14, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants