? Success

User tests: Successful: Unsuccessful:

avatar Hackwar
Hackwar
10 Feb 2015

This sets the frontediting to not enabled by default for performance reasons. If someone needs this feature, they should enable it specifically.

This also includes the changes from #6032, so that #6032 can be merged regardless of the discussion around this here.

avatar Hackwar Hackwar - open - 10 Feb 2015
avatar joomla-cms-bot joomla-cms-bot - change - 10 Feb 2015
Labels Added: ?
avatar waader
waader - comment - 10 Feb 2015

@test works for me!

@brianteeman When testing this patch at first I could not find the respective configuration setting. Then I looked in the code and found it behind the term "Mouse-over Edit Icons for". I always watched out for "Frontend"-something.

Also the option terms were not clear to me. It states "Module" when I could also edit mod_menu and "Modules & Menus" when mod_x and Menu Items are meant. So "Modules & Menu Items" would be more precise.

avatar brianteeman
brianteeman - comment - 10 Feb 2015

That is off topic for this issue

On 10 February 2015 at 11:38, waader notifications@github.com wrote:

@test https://github.com/test works for me!

@brianteeman https://github.com/brianteeman When testing this patch at
first I could not find the respective configuration setting. Then I looked
in the code and found it behind the term "Mouse-over Edit Icons for". I
always watched out for "Frontend"-something.

Also the option terms were not clear to me. It states "Module" when I
could also edit mod_menu and "Modules & Menus" when mod_x and Menu Items
are meant. So "Modules & Menu Items" would be more precise.


Reply to this email directly or view it on GitHub
#6041 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

avatar Hackwar
Hackwar - comment - 10 Feb 2015

but it might be good to fix the wording prior to 3.4.0 and add a post install message that gives notice regarding the performance hit when this is enabled.

avatar waader
waader - comment - 10 Feb 2015

@brianteeman I don´t think so because this feature is already advertised and some people will want to try it out. When it´s activated by default, you will not have to find a configuration setting. Otherwise you have to. Anyway.

avatar brianteeman
brianteeman - comment - 10 Feb 2015

I guess you didnt realise then that this text has been present since 3.3 at
least. The change is that the behaviour is now to open the module in the
frontend not the backend.

On 10 February 2015 at 11:57, waader notifications@github.com wrote:

@brianteeman https://github.com/brianteeman I don´t think so because
this feature is already advertised and some people will want to try it out.
When it´s activated by default, you will not have to find a configuration
setting. Otherwise you have to. Anyway.


Reply to this email directly or view it on GitHub
#6041 (comment).

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

avatar Bakual
Bakual - comment - 10 Feb 2015

Be aware that this paremeter isn't a new feature and it already exists already in 3.2.
Changing the default value thus will have an impact on existing sites and remove the frontend editing for sites where it was enabled before.
That is in the case where the global configuration wasn't saved since 3.2.0.

Thus it isn't a backward compatible change. Although minor.

avatar Hackwar
Hackwar - comment - 10 Feb 2015

It is not something that breaks a site. It also isn't something that changes anything for Google or any normal user. You have to have quite some permissions to use this feature. Regardless of what we do with this PR, we should add a post install message telling people of the very negative effect this feature has on their performance.

avatar brianteeman
brianteeman - comment - 10 Feb 2015

I thought With your latest pr there is no negative effect
On 10 Feb 2015 13:50, "Hannes Papenberg" notifications@github.com wrote:

It is not something that breaks a site. It also isn't something that
changes anything for Google or any normal user. You have to have quite some
permissions to use this feature. Regardless of what we do with this PR, we
should add a post install message telling people of the very negative
effect this feature has on their performance.


Reply to this email directly or view it on GitHub
#6041 (comment).

avatar Bakual
Bakual - comment - 10 Feb 2015

Please not yet another postinstall message.
And we indeed should fix the negative effect (which the other PR should do).

avatar Hackwar
Hackwar - comment - 10 Feb 2015

As I wrote, that PR only solves the issue for unauthenticated users, users that are not logged in. All users that ARE logged in, will still have all these checks and there is nothing that we can do against that, except switching off the feature. Which is why I'm proposing to setting this to not enabled by default. For all community websites, this will still mean quite a performance hit. For all webshops where you login at some point, this still means a performance hit. So, no, #6032 does NOT fix this issue. It fixes this for all static websites out there, but the issue remains. Which is why I proposed this PR. And which is why I proposed to not allow to select this for single modules, but all or nothing.

P.S.: Did anybody notice how I always used "not enabled" instead of "disabled"? I fear that this gets shot down so much just for the language that I use, that I desperately try to use positive words, because otherwise the B/C-police will call to the rescue again to help all those 5 poor souls that need this enabled, but can't find the option in the global configuration. I mean, of course it is better to reduce the speed of 99% of all websites made with Joomla out there instead of making that 1% that might be using it search for that option.

And of course we should tell people that we pushed a feature into Joomla that is enabled by default, that slows down their site, that they are most likely not using and that can be switched off by setting it like that in the global configuration.

avatar Hackwar
Hackwar - comment - 10 Feb 2015

Just to make sure that no one misses this:

Neither #6032 nor #6041 solves the real issue! We would have to remove the feature to really fix it. They both just ease the pain that is introduced by this thing.

avatar chmst
chmst - comment - 10 Feb 2015

Watching your discussions for a while - and highly interested in usability and performance and clean code I want to give my2cent. Wouldn't it be better to say frankly "For a better performance we have decided to disable this feature - if you really need it - be aware that it affects the performance". Users are notas supid - they know that bad performance is bad for their sites.
Backward compatibility is important, indeed. This makes it difficult. But I'm sure that you can communicate this.

avatar nternetinspired
nternetinspired - comment - 11 Feb 2015

Watching your discussions for a while - and highly interested in usability and performance and clean code I want to give my2cent. Wouldn't it be better to say frankly "For a better performance we have decided to disable this feature - if you really need it - be aware that it affects the performance". Users are notas supid - they know that bad performance is bad for their sites.

:100:

Improved performance is also an important feature, one that many would rate higher than front-end editing.

avatar zero-24 zero-24 - change - 11 Feb 2015
Easy No Yes
avatar zero-24 zero-24 - change - 12 Feb 2015
Category Front End Modules
avatar zero-24
zero-24 - comment - 16 Feb 2015

@Hackwar since this is RTC: #6032 do we still need this change here or can we close this PR?


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6041.
avatar Hackwar
Hackwar - comment - 17 Feb 2015

Please see my comment above. This PR is still definitely needed.

avatar Bakual
Bakual - comment - 17 Feb 2015

This PR still isn't B/C.

avatar Hackwar
Hackwar - comment - 17 Feb 2015

I strongly disagree. By your logic, the recaptcha plugin would not be B/C either. We are disabling a feature on websites that did NOT enable it in the first place. So in fact, you forced this on people somewhere in 3.2 and now I'm proposing to let people choose instead. It does not break anything and it does not take a feature away. It only makes 95% of all current Joomla websites 20% faster.

avatar Bakual
Bakual - comment - 17 Feb 2015

Why would the recaptcha plugin not be BC? It defaults to the old API afaik.

Yes, we had that feature enabled by default, which was probably wrong back then. But you propose to disable it now while it worked for all sites during the last two minor releases.

It only makes 95% of all current Joomla websites 20% faster.

That's absolutely not true. With your other PR it only affects sites where a user is logged in. Which isn't 95% of the Joomla installations for sure and by far.
The performance increase is also not 20%. The reported increase is 20% for the database queries, which is not the full website. On my crappy testing server at home, it accounts for about 2% performance gain. It obviously will have different gains depending on server, caching and website.
Together with the other improvements, it will be an even smaller gain.

So please don't try to get this PR merged by using false facts. That will not help your case.

avatar Hackwar
Hackwar - comment - 17 Feb 2015

Let me give you another example: We changed the password hashing method in between minor releases, even worse, we changed them again in a patch release. That actually broke extensions and several websites and it broke them in the worst possible way. That apparently was okay. Fixing this for some reason is not okay, even though it does not break any extension, any website or any feature.

The performance improvement when this is disabled on my system is a lot bigger than 2%. I didn't calculate it exactly, but remembering the numbers it should be somewhere around 10%. However, the exact amount doesn't matter one bit.

What matters is, that this feature, the way it is, adds noticeable load on websites and even with my fix in #6032, this additional load is still present for all logged-in users. Yes, you can switch this feature off and everything would be fine, but you would have to first of all know about this feature and that it has a negative impact on your performance and then know how to switch it off.

Now, I claim for myself that I know Joomla pretty well. I used it here or there a tiny, teensie bit and might have read a few anouncenments and a bit of source code [sarcasm off] and I had NO idea that this feature had negative performance effects for everybody and I didn't know that you could switch it off. And even after I investigated the source of the performance problem described in #5996 and provided a preliminary fix for it in #6032, I still didn't know where to switch this off. I had to look into the code of com_config to learn that "Mouse-over Edit Icons for" means that I can edit modules in the frontend. The description "Select if you want mouse-over edit icons for modules and menu items (support may depend on your template)." is equally useless. The german label is equally confusing, while the description is at least a tiny bit better, but you would still have to know quite a bit about the system to make anything good out of that. The french translation is mostly identical to the english text.

So, again, there is a feature that people know very little about, that is currently broken in 3.3 in terms of performance, where we only have a bandaid solution for 3.4 and whose off-switch is hidden so good that we might as well remove it altogether and nobody would notice.

This is not a break in backwards compatibility. That would assume that something is broken after this change. That is the definition of backwards compatibility and breaking it. Here we are talking about a change in functionality. These happen all the time and are expected. So please don't claim that this is a break in compatibility.

avatar Bakual
Bakual - comment - 17 Feb 2015

That apparently was okay.

Not it wasn't. And it was one of the reason we changed our whole strategy.

The performance improvement when this is disabled on my system is a lot bigger than 2%. I didn't calculate it exactly, but remembering the numbers it should be somewhere around 10%.

As said, it may differ a lot between enviroments. Depending on how fast the database is, the harddisc, the PHP stack, how many modules there are, how many other database queries from extensions, stuff like that. But it's certainly not 20%. On the other hand I agree it's noticable on most websites where users are logged in.

That is the definition of backwards compatibility and breaking it.

That may be your definition. It's certainly not mine.
Taking your statement that this switch is so well hidden, how do you expect the users who used this feature now for two minor release to find the switch to enable it again?

In my opinion it's a backward compatibility break and if we are going to make an exception and merge this, I want people to be aware that this will cause support requests for sure.
I would like to have the other possibilities (like your other PRs) tested and merged first.
If the performance issue is still big enough after all the other improvements, then I'll leave it up to @wilsonge to make that decision to create a minor B/C here.

avatar Hackwar
Hackwar - comment - 17 Feb 2015

So because the language string is total bullshit, we can't change this? I'd rather create yet another PR to change that language string to something sensible, than to drop this PR. I also already proposed a post-install message that makes people aware of this.

The argument "Our interface does not label that switch correctly, so we can't set it to the value that it should be" is no argument for me.

avatar Hackwar
Hackwar - comment - 17 Feb 2015

Again, going by the logic that has been applied so far to this setting, we should not set it to "1" as default, but "2", because otherwise people don't know that they can edit menu items on the frontend, too.

#6105 attempts to improve the strings.

avatar Bakual
Bakual - comment - 17 Feb 2015

The argument "Our interface does not label that switch correctly, so we can't set it to the value that it should be" is no argument for me.

It was your argument that the switch is hard to find. Not mine.

avatar Hackwar
Hackwar - comment - 17 Feb 2015

No, my argument is, that the switch is so hidden and impossible to find, that even if people knew about this feature and the consequences of this feature, that they were unable to disable it. You drew the conclusion that we could not disable it then, because people would be unable to enable it again. The same reason that I would buy a new car every 2 months because the ashtray is full if I were smoking. The correct conclusion would be to make it simple for people to find the switch by improving the label and adding a post-install message.

avatar KingLouis1 KingLouis1 - test_item - 24 Oct 2015 - Tested successfully
avatar KingLouis1
KingLouis1 - comment - 24 Oct 2015

I have tested this item :white_check_mark: successfully on f0267e3

Very difficult to find the functionallity, but finally it worked


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

avatar svom svom - test_item - 24 Oct 2015 - Tested successfully
avatar svom
svom - comment - 24 Oct 2015

I have tested this item :white_check_mark: successfully on f0267e3

Tested successfully

Joomla 3.4.5
Chrome


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

avatar widmann-it
widmann-it - comment - 24 Oct 2015

how can I get access again . under which menu item . so not ok may be given

avatar wilsonge
wilsonge - comment - 24 Oct 2015

It's an option in global configuration

avatar widmann-it
widmann-it - comment - 24 Oct 2015

we still have the icon at the postion " Recent Posts " Archive Articles etc. The patch works only with items.

The menu item for the A and Blend is very confusing deposited . Does not quite fit . I would put it under Article Options purely

avatar zero-24 zero-24 - change - 24 Oct 2015
Milestone Added:
Status Pending Ready to Commit
avatar zero-24 zero-24 - change - 24 Oct 2015
Milestone Added:
avatar zero-24
zero-24 - comment - 24 Oct 2015

RTC :smile:


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

avatar joomla-cms-bot joomla-cms-bot - change - 24 Oct 2015
Labels Added: ?
avatar rdeutz rdeutz - change - 24 Oct 2015
Milestone Added:
avatar rdeutz rdeutz - change - 24 Oct 2015
Milestone Removed:
avatar roland-d
roland-d - comment - 25 Oct 2015

Let's make a decision on this, I am going to ask the PLT to vote on whether or not to accept this PR. If it is accepted I will merge it for 3.5 otherwise I will close it.

avatar wilsonge wilsonge - change - 6 Nov 2015
Milestone Removed:
avatar roland-d roland-d - change - 14 Nov 2015
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2015-11-14 20:44:10
Closed_By roland-d
Labels
avatar roland-d roland-d - change - 14 Nov 2015
Labels
avatar roland-d roland-d - close - 14 Nov 2015
avatar joomla-cms-bot joomla-cms-bot - close - 14 Nov 2015
avatar roland-d
roland-d - comment - 14 Nov 2015

The PLT has decided not to change this behavior but that the language string needs updating. I have made a suggestion for that in the PR #6105

Thanks everybody for your contribution.


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

avatar joomla-cms-bot joomla-cms-bot - change - 14 Nov 2015
Labels Removed: ?

Add a Comment

Login with GitHub to post a comment