Skip to content

Commit

Permalink
Update behavior.php
Browse files Browse the repository at this point in the history 
Second parameter in [script] function should be now [false] for [tabstate] function. This is because this tabs-state.js requires JQuery not full Mootools framework. Setting this to [true] not only load Mootools when it is not required but also cause some conflicts if websites that depends on JQuery without noConflict.
  • Loading branch information
@artur-stepien
artur-stepien committed Nov 8, 2013
1 parent 153ee41 commit 0247d9f
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion libraries/cms/html/behavior.php
View file
Expand Up @@ -804,7 +804,7 @@ public static function tabstate()
}
// Include jQuery
JHtml::_('jquery.framework');
JHtml::_('script', 'system/tabs-state.js', true, true);
JHtml::_('script', 'system/tabs-state.js', false, true);
self::$loaded[__METHOD__] = true;
}
}

23 comments on commit 0247d9f

@cyrezdev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing to 'false' makes 'Install from web' broken on "loading..."
And Mootools still loaded, so no gain.

@Bakual
Copy link
Contributor

@Bakual Bakual commented on 0247d9f Dec 18, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing to 'false' makes 'Install from web' broken on "loading..."

The bug is in the "Install from Web" plugin and needs to be fixed there (made a PR on their repo already)

And Mootools still loaded, so no gain.

We're working on it, step by step 😄

@artur-stepien
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Bakuall is right. Thats "Install from Web" bug. Probably they didn't attached mootools and used that included by tab-state what is bad idea.

@Bakual
Copy link
Contributor

@Bakual Bakual commented on 0247d9f Dec 18, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They didn't need mootools, but they need the Joomla namespace which is defined by core.js which itself is loaded together with (but not part of) the Mootools framework.
See joomla-extensions/install-from-web-client#1 for the PR 😄

@artur-stepien
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So shouldn't those two (Mootools and Joomla namespace) be disconnected from each other? Does core.js requires Mootools?

@Bakual
Copy link
Contributor

@Bakual Bakual commented on 0247d9f Dec 18, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, it requires mootools, but there is a PR around which will change this.
I'm working on adding a new method so we can load core.js independantly from behavior.framework (aka Mootools).

@beat
Copy link
Contributor

@beat beat commented on 0247d9f Dec 18, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR does not fullfil its intention as mootools is still loaded, but from the help button.

Additionnaly, it is a backwards-compatibility issue and we should not create knowlingly a backwards-compatibility issue in a dot release. It's just wrong towards the poor joomla user.

It's a great addition, but it really should not be in 3.2.1 and should wait for 3.3 (and be completed with other PRs that allow to not load at all mootools in the backend).

This PR will for sure break hundreds of extensions. Joomla should not want that in a dot-release per semver.org standards.

From a Joomla user perspective (and that should really be our main focus) I'm recommending to revert this PR merge for Joomla 3.2.1, and to require a new one for Joomla 3.3.

@artur-stepien
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You talk now about my PR or Bakual's?

@cyrezdev
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beat : From a Joomla user perspective (and that should really be our main focus) I'm recommending to revert this PR merge for Joomla 3.2.1, and to require a new one for Joomla 3.3.

Too late, Joomla 3.2.1 is released...

And Bakual's PR seems to work (it does for me) so, with other more successfull tests, do a release of plugin in 1.0.5 as soon as possible ?
No ?...

@beat
Copy link
Contributor

@beat beat commented on 0247d9f Dec 19, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, actually when I raised this issue, it wasn't too late, but the regular Joomla users hassles didn't get priority.

Now Install from Web is broken and a new version has been released in emergency mode.

Moreover, as even the initial load got broken by Joomla 3.2.1 (and this patch), the users can't simply click the nice little button they normally have in the install from web tab, but have to guess that they need to go to Updates and do an extensions update to get it fixed.

All due to an uncordinated change and lack of taking in account feedbacks from 3.2.1 RC-testers who reported the issue in time.

Probably a few hundred of extensions that are expecting var Joomla = {}; to be defined by a Joomla "API" will break too.

Anyway, life goes on. I did what I could do to warn, now the Install from Web team did in emergency what we could do to limit damage by making the Install From Web plugin upgrade available within hours of discovering that 3.2.1 got released without reverting this PR, as a mandatory upgrade for everybody. That way hopefully, they will upgrade that before Joomla and be able to upgrade the easy way.

As said, life goes on. Merry Christmas and Happy New Year.

@artur-stepien
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As all bugs do this also have some bad results. But in my opinion it is better to feels some pain for a while and get problem fixed before it get bigger. It could be done better but it is fixed and we all should be happy that bug was found.

@beat
Copy link
Contributor

@beat beat commented on 0247d9f Dec 19, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, how do we fix the regression for next Joomla 3.2.2 release ?

I would like to see this solved now, so it can get thoroughly tested and merged well in advance, and 3.2.2 might be sooner or later, who knows...

@Bakual
Copy link
Contributor

@Bakual Bakual commented on 0247d9f Dec 19, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To reiterate:
There was no BC break at all in an API.
The API for loading MooTools and core.js (and thus the Joomla namespace) is JHtml::('behavior.framework'). It always has been and still is.

The only problem we had is that the plugin didn't use the API at all to load core.js. It just wrongly assumed it's loaded always somehow. This never was the case. Each extension has to load it when they want to use it. That was always and is still the case.

Yes, tabstate no longer loads the full mootools stack and core.js. But the API never said it does. It's supposed to save the active tab in the session, and it still does that. No API break here. Also the plugin didn't make a call on tabstate neither.

So how are you talking about a BC break in an API when in fact the plugin didn't use an API at all?

@beat
Copy link
Contributor

@beat beat commented on 0247d9f Dec 19, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Thomas,
You can write in bold or as big as you want. It will not change the facts:

Fact is that 3.2.1 introduced a regression in something that was working, and made a javascript variable that was called "Joomla" disappear. If "Joomla" isn't an API (yes, a Javascript API, not a PHP one), then I don't know what an API is, sorry. And it was a given to assume safely in 3.x until 3.2.1 that this API was available within a tab in the admin area.

If your goal is to point finger to the guilty vs the non-guilty, that's a topic I am not interested in, as it never leads to a constructive discussion.

So to be clear: Ok, if that makes you feel better, in the Apps* team (mea culpa too), we made 2 (obviously wrong) assumptions:

  1. That the Joomla basic APIs in PHP and Javascript were a given within a core tab of Joomla (e.g. Javascript Joomla-API main variable was present always in backend in a tab).
  2. That Joomla PLT would respect the backwards compatibility of its own extensions at least a slight bit better than that of any other third-party extension in case our assumption number 1 above was wrong.

For "1", obviously, we were WRONG: Joomla willingfully stopped exposing its Javascript API in a tab of the admin area.

For "2.", here too obviously, we were WRONG: Joomla PLT willingfully and despite at least 4 testers alerts/reports on that broken feature during RC tests 44 hours before release, and alerts from me and from another major Joomla extensions developer, has chosen to not fix the side-effect of this PR and instead to blame its install from web plugin team and to impose a new bug to its user-base, without an obvious fix to go to "Extensions Updates".

Fact is that Joomla PLT for Joomla 3.2.1 willingfully kept its first own independant extension broken by the 3.2.1 upgrade despite many reports from RC-testers.

It feels like the left hand of Joomla is biting its right hand (or the other way around, that's not the matter), ignoring the pain of the bite.

And now, instead of just discussing the technical details on how to fix that breaking for 3.2.2 so that less people get affected by that incompatibility, you are again pointing fingers.

Are we working as developers for the interest of the Final Truth of APIs, or are we working in the best interest of Joomla (and thus of its Joomla Users) ?

I just don't get it.

@Bakual
Copy link
Contributor

@Bakual Bakual commented on 0247d9f Dec 19, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fact is that 3.2.1 introduced a regression in something that was working, and made a javascript variable that was called "Joomla" disappear.

This is exactly where you are wrong. The variable didn't disapear. It's in fact even loaded in the install view. Just no longer before the plugin is. If the plugin had used the API, all would be fine. And it most likely also will stay on most pages because it's needed by the view. It may however be missing in future releases if it's not needed by the view. Doesn't make sense to load an extern javascript file if it's not needed, right?

Fact is that Joomla PLT for Joomla 3.2.1 willingfully kept its first own independant extension broken by the 3.2.1 upgrade despite many reports from RC-testers.

As soon as I noticed this issue, I wrote a PR to fix it. It could of course have been fixed earlier, if someone wrote the fix earlier. That's true.
But delaying the release because this plugin was broken? A plugin, which has to be updated independently from the CMS anyway? Keep in mind the issue wasn't a "major" or "ciritical" issue which would justify a delay of the release. It was a "normal" issue. You could still install and manage extensions without any problems and in frontend nothing was broken at all.

And now, instead of just discussing the technical details on how to fix that breaking for 3.2.2 so that less people get affected by that incompatibility, you are again pointing fingers.

As soon as you point me to the real issue or API break, we can look for a solution.
By the way, I already proposed a solution to the problem we will be facing when core.js is rewritten to jQuery (probably with the next minor release) as we need an API to load core.js without MooTools by then. See #2696, feedback is welcome there.

@beat
Copy link
Contributor

@beat beat commented on 0247d9f Dec 19, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fact is that 3.2.1 introduced a regression in something that was working, and made a javascript variable that was called "Joomla" disappear.

This is exactly where you are wrong. The variable didn't disapear.

  1. Well, if you say that I am wrong that we had something that was working that stopped working, as evidenced here and here and here:
    http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=32981&start=0
    http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=32992&start=0
    http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=32987&start=0
    then there is no point in continuing this discussion.
  2. And yes, the variable did disapear from the scope of where it was available. Technically the variable didn't disappear from source code either :-D , let's not play on words, please. ;-)

Thanks for your decoupling PR on which I commented: #2696 (comment) that it won't solve this B/C incompatibility unless that or another PR makes it included in admin area or at least in tabs.

Another way to handle that properly would have been to deprecate that variable from that in-tabs scope, give time to fix the plugin and other extensions making obviously "wrong" assumptions, then remove it without implications to the user.

Even putting back the var Joomla = {}; with the safeguard if around it into the core or even in the installer to avoid the incompatibility introduced as side-effect of this PR would have not delayed the release of 3.2.1 by more than 15 minutes, and would have been respectful to the Joomla users.

Merry Christmas!

@artur-stepien
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now its time stop blaming and and move on. This discusson is not only waste of time that could be used more productive but also makes unnecessary noise around bug that is already fixed.

@Bakual
Copy link
Contributor

@Bakual Bakual commented on 0247d9f Dec 19, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never said the plugin didn't stop working. I just said the issue is not withing the CMS.
The variable is still there and core.js is loaded. It's just a matter of the order when it's loaded. If the plugin would load its dependencies, core.js would still be loaded before the plugin JavaScript code.

Another way to handle that properly would have been to deprecate that variable from that in-tabs scope

Core.js isn't needed at all for the tabs.

Even putting back the var Joomla = {}; with the safeguard if around it into the core or even in the installer

This exact code is loaded in the install view. That's what I'm trying to say the whole time. We didn't remove anything. The API still is there and is even loaded in the installer. All that happend is that your plugin got loaded before core.js now.

@beat
Copy link
Contributor

@beat beat commented on 0247d9f Dec 19, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Bakual i'm not discussing in the empty anymore here.

My only question: Will PLT accept a PR to core com_installer to create the global Joomla variable for having a workaround for people with plugin 1.0.4- (versions without the fix) and Joomla 3.2.2+ ?

@Bakual
Copy link
Contributor

@Bakual Bakual commented on 0247d9f Dec 19, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do such a PR, please use the API and use JHtml::_('behavior.framework').

@beat
Copy link
Contributor

@beat beat commented on 0247d9f Dec 19, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, that's starting to be constructive. Would PLT accept such a PR (supposed that it's passing usual testing) ?

@Bakual
Copy link
Contributor

@Bakual Bakual commented on 0247d9f Dec 19, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably yes.

@mbabker
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would PLT accept such a PR (supposed that it's passing usual testing) ?

Does it introduce any regressions? Does it resolve an outstanding issue? Is it an enhancement to the stability of the core code? Is it reliable code that has a use in the core of Joomla? Just typical questions that should be able to be answered when looking at a PR. If they're in the positive, it is difficult to turn down code (for me anyways).

Please sign in to comment.