? Success

User tests: Successful: Unsuccessful:

avatar nicksavov
nicksavov
1 Nov 2013

The issue is that the version icons still appear, even though content history is off by default when upgrading from Joomla 3.1.5. The simple solution is to hide the icons by default if the feature is not enabled.

Special thanks to Michael for helping me find the code for the change and also for bouncing my idea(s) off of him for advice.

For more info, see discussions at:
http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=32348

and:
http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=32332

avatar nicksavov nicksavov - open - 1 Nov 2013
avatar beat
beat - comment - 1 Nov 2013

Code review: Looks good, like a safe change, code-wise (as long as they are off by default ;-) ).

avatar Bakual
Bakual - comment - 1 Nov 2013

I already did a PR for this a week ago. See #2337

While you only changed the defaults in the views, I also made it by default turned off in the config.

avatar mbabker
mbabker - comment - 1 Nov 2013

I don't think we want to by default disable versioning for everybody, and truth be told, even with my responses on that other PR I forgot it was there with everything going on. But, it does need to be consistent in at least the PHP because as it stands right now, upgraders will see the version toolbar button without the rest of the versioning checks being enabled.

avatar Bakual
Bakual - comment - 1 Nov 2013

I thought for new installs the settings will be saved using the SQL files. The default values in the config xml files should not matter there as far as I understand it. Or am I wrong here?

avatar mbabker
mbabker - comment - 1 Nov 2013

We're at such a point of inconsistency with our own code base, it's a massive headache to manage anything anymore...

So, yes, the core components all have a default config set up for install which basically invalidates every other default in the XML forms or PHP checks.

avatar Bakual
Bakual - comment - 1 Nov 2013

@mbabker I certainly agree with the statement about inconsistency :)

Imho, default values in PHP code and XML files should match everywhere to avoid unpredictable behaviours for updaters.
New installs are threated differently anyway as we set the (different) values there like we feel they are useful.

avatar beat
beat - comment - 1 Nov 2013

@Bakual :+1: Agree that PHP and XML defaults MUST match otherwise they get changed at next save!

Suggestion here: Why not by default ENABLE the versioning everywhere but just DISABLE globally by default until we are sure about B/C in 3.2.2... Then we can ENABLE default in global setting.

It's much simpler for people to turn it on, PLUS we can have it ON by default in new installs.

Thoughts?

avatar Bakual
Bakual - comment - 1 Nov 2013

See other PR why enabling it by default will not work.

avatar beat
beat - comment - 1 Nov 2013

@nicksavov :+1:

@Bakual Yes, it needs to be consistent accross the board. It is a pain (see number of duplicate lines that need changes) because the param is not tested in an Observer centrally, and that all those layouts are not simply generated by the Observer centrally. Something that could be refactored in a future release.

avatar nicksavov
nicksavov - comment - 3 Nov 2013

Good catch, Thomas! #2337 was merged, so I'm closing this one.

Cheers

avatar nicksavov nicksavov - close - 3 Nov 2013

Add a Comment

Login with GitHub to post a comment