? Success

User tests: Successful: Unsuccessful:

avatar Kubik-Rubik
Kubik-Rubik
25 Feb 2015

See #6171 "When content languages have no set access level, it will break peoples sites with 3.4.0."

Problem also described in the forum: http://forum.joomla.org/viewtopic.php?f=707&t=874568

avatar Kubik-Rubik Kubik-Rubik - open - 25 Feb 2015
avatar joomla-cms-bot joomla-cms-bot - change - 25 Feb 2015
Labels Added: ?
avatar Bakual
Bakual - comment - 25 Feb 2015

Unfortunately this will not work for everyone.
The value 1 is only public by default. On the users sites, it may have a totally different value. 1 may be a protected access level or don't exist at all.

The only safe way I see would be to add code which deals with the case that the value is either null or 0, which means it was never set. We can safely assume this means public. We can mark this code deprecated for 4.0 already when adding.
To make sure the user saves the parameter before Joomla 4.0, we can add a postinstall messages which only shows up if the value isn't saved. The action button would then load the parameters.

avatar mbabker
mbabker - comment - 25 Feb 2015

If the value is 0 I think it's a pretty safe assumption that the content language was public to begin with. IMO, this is the simplest one time fix and does not create a complicated code solution. With that said, it would fail if someone deleted the group, so maybe we just need a subquery to get the minimum ID from the #__viewlevels table, but I would really suggest trying to avoid a PHP based solution here when we don't need it.

avatar Bakual
Bakual - comment - 25 Feb 2015

It would also fail if you get the minimum ID from the table. The viewlevels are not ordered by ID. The viewlevel "Public" may even be a restricted level which is named stupid. We just don't know that.
So there is no way to solve it in the database since we don't have (and can't get) the needed information.
We can only tell the user to save the correct value and/or adjust the code to work without a value (0 / null) as well.

avatar mbabker
mbabker - comment - 25 Feb 2015

Adjusting the code to work with a 0/null value just introduces buggy behavior. For all intents and purposes, the access column is supposed to act as a foreign key, but since we refuse to introduce/use them in core, the only thing we can do is enforce correct values.

avatar Bakual
Bakual - comment - 25 Feb 2015

Adjusting the code to work with a 0/null value just introduces buggy behavior.

To my knowledge it was the behaviour till now.

avatar Hackwar
Hackwar - comment - 25 Feb 2015

No, the behavior up till now was to not care for access levels at all. This PR is a perfect solution and everything else is simply trying to make stuff more complicated than necessary. If I didn't know better, I could come to the idea that the PLT is desperately trying to keep my code out of Joomla. What do you think the alternative is? Reverting #5140? In that case, I have to close my other 10 PRs and stop any work on routing and declare the crowdfunding campaign a failure.

avatar mbabker
mbabker - comment - 25 Feb 2015

Either way, in a PROPERLY stored data record, a 0/null value for the access column is an invalid value and the data has been invalid since the day it was recorded to the database. Accepting it in the code means that we are allowing a broken data set to live in our user's databases instead of attempting to address the invalid data. If proper foreign keys were used, the save would fail because of a constraint violation.

avatar PhilETaylor
PhilETaylor - comment - 25 Feb 2015

If I didn't know better

You do - so stop making this personal and stick to the code...

Can someone just merge this so we can get on with our lives... there really is bigger fish to fry....

In that case, I have to close my other 10 PRs and stop any work on routing and declare the crowdfunding campaign a failure.

Really... threats?... I thought you were bigger than that...

avatar mahagr
mahagr - comment - 25 Feb 2015

I would rather deal issues with minority of the sites than all the old installations. If nobody finds better solution, I would think of using this one, which at least fixes most of the issues.

We could implement better heuristics on figuring out the public access level, but would it be faster to fix those few instances manually where the issues persist after this fix?

avatar phproberto
phproberto - comment - 25 Feb 2015

:+1: here

In the worse case we will be assigning a non-public group which is not a security issue and they will still have the issue. I would rather develop a custom script to fix those sites that may be broken than complicating this fix.

avatar alikon
alikon - comment - 25 Feb 2015

despite in theory i agree with @Bakual , in italy we use say better injuried than dead
in practice i would be in favour of this fix as a temporary workaround

avatar Bakual
Bakual - comment - 25 Feb 2015

Just so we're aware of what "workaround" code I am talking about:
The breaking change is most likely this line: a9c7eb2#diff-9a4ee24a09bcef172864692ed05135caL82. I couldn't test it yet but I'd guess reverting that single check would fix the issue for all sites. Or am I wrong with that assumption?

avatar infograf768
infograf768 - comment - 26 Feb 2015

@Bakual
I confirm your assumption, as I had stated on bugsquad chat:

[25/02/15 11:30:40] infograf768: ah, you took off
if (isset($language->access)
I should ahve looked better

Reverting that line solves the issue.

avatar infograf768
infograf768 - comment - 26 Feb 2015

here is a PR for it
#6194

avatar mahagr
mahagr - comment - 26 Feb 2015

Yes, it looks like the breaking change to me. Of course isset() isn't needed as value should always be present.

Dealing 0 would indeed restore the old behavior, but it looks like most of us do not want the old hack back, but will rather force users to deal with broken data once and for the good.

As a compromise I would accept reverting the check if there will be a sticky notification in the backend to ask users to fix the data as long as value 0 is found in the table. Either that or this fix...

avatar Bakual
Bakual - comment - 26 Feb 2015

As a compromise I would accept reverting the check if there will be a sticky notification in the backend to ask users to fix the data as long as value 0 is found in the table.

We can add that postinstall message easily. But it doesn't need to be done together with the revert if nobody has time before we release 3.4.1. It's not a blocker.
I agree such a message would be perfect, as otherwise we will face the same issue again in future.

avatar infograf768
infograf768 - comment - 26 Feb 2015

closing in favour of #6194

avatar infograf768 infograf768 - change - 26 Feb 2015
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2015-02-26 07:52:55
avatar infograf768 infograf768 - close - 26 Feb 2015
avatar infograf768 infograf768 - close - 26 Feb 2015
avatar Hackwar
Hackwar - comment - 26 Feb 2015

I disagree with closing this one. It is still a very necessary step. Otherwise we have this same issue at another time.

avatar infograf768
infograf768 - comment - 26 Feb 2015

if a postinstall message is created, we should not need this.

avatar wilsonge wilsonge - reference | ed606a4 - 27 Feb 15
avatar Kubik-Rubik Kubik-Rubik - head_ref_deleted - 20 Jun 2015

Add a Comment

Login with GitHub to post a comment