User tests: Successful: Unsuccessful:
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
Labels |
Added:
?
|
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.
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.
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.
Adjusting the code to work with a 0/null value just introduces buggy behavior.
To my knowledge it was the behaviour till now.
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.
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.
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...
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?
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.
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?
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...
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.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2015-02-26 07:52:55 |
I disagree with closing this one. It is still a very necessary step. Otherwise we have this same issue at another time.
if a postinstall message is created, we should not need this.
Unfortunately this will not work for everyone.
The value
1
is onlypublic
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
or0
, which means it was never set. We can safely assume this meanspublic
. 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.