CONTENIDO / CONTENIDO

CONTENIDO CMS
Other
17 stars 12 forks source link

[Bug] Category state color in "Articles" tab is wrong under certain circumstances #405

Open tone2tone opened 1 year ago

tone2tone commented 1 year ago

With the latest dev-version installed, some categories are show in red color even though they have an online start article. As far as I can see, this happens with categories that hold more than one article. It depends on the order of the articles on how the system decides as to what state the category is. If the first article in the list is online, the second offline, then the category turns red as if "offline". If the first article is offline, the second one online, then the category is marked as "online". We had such wrong behaviour in an earlier version before.

muratpurc commented 1 year ago

The current logic to display a red category entry in the article overview is as follows:

I've checked this with differrent article states (on/offline start article/no start article) and couldn't force a wrong behaviour.

Do you have more details to reproduce the issue?

Addendum: Changing the state of an article doesn't reload the article overview, you have to reload the article overview manually in order to see the changed category symbols.

The CSS class for categories marked red is set in the file contenido/includes/include.con_str_overview.php in lines 128-133.

Whether the category has a start article or an online article is indicated in the same file between the lines 661-670.

tone2tone commented 1 year ago

Hi Murat, I know most of the things like the refresh thing and such. The only thing I can say is that I am currently updating a multitude of Contenidos (always clean install, swaping contenido folders and such), and I do it a lot. Even with the same hosting provider and PHP 8.1 running everywhere and doing the exact same update process, in some installations, I end up with the COMPLETE wrong category colors in the articles tab. In some installations, everything is fine and I can't reproduce it there either, but in others, it is a nightmare. I have no log entries to show, no indication so far as to how and where this bug comes from. The class is set to "on_error". Only if I put ALL articles in a category online, the category is shown as online as well. If i put ANY of the articles offline, the category turns back to red.

muratpurc commented 1 year ago

This is very strange behavior and the possibility that the display of the category colors may be incorrect cannot be completely ruled out.

As previously written, I couldn't reproduce it for myself. The problem would have to be analyzed and isolated in more detail on a CONTENIDO installation where it occurs.

tone2tone commented 1 year ago

Okay, found it. In contenido/includes/include.con_str_overview.php, two statements are missing in the if-clause starting at line 439. The WHERE-element of both SELECT statements in if/else needs one more line that was dropped from the previous versions. The statement a.online = 1 AND has to added, so that the whole section reads:

if ($syncoptions == -1) {
    $sql2 = "SELECT
                c.idcat AS idcat,
                SUM(a.online) AS online,
                d.startidartlang
            FROM
                " . $cfg["tab"]["art_lang"] . " AS a,
                " . $cfg["tab"]["art"] . " AS b,
                " . $cfg["tab"]["cat_art"] . " AS c,
                " . $cfg["tab"]["cat_lang"] . " AS d
            WHERE
                a.idlang = " . cSecurity::toInteger($lang) . " AND
                a.idart = b.idart AND
                b.idclient = '" . cSecurity::toInteger($client) . "' AND
                a.online = 1 AND
                b.idart = c.idart AND
                c.idcat = d.idcat
            GROUP BY c.idcat, online, d.startidartlang";
} else {
    $sql2 = "SELECT
                c.idcat AS idcat,
                SUM(a.online) AS online,
                d.startidartlang
            FROM
                " . $cfg["tab"]["art_lang"] . " AS a,
                " . $cfg["tab"]["art"] . " AS b,
                " . $cfg["tab"]["cat_art"] . " AS c,
                " . $cfg["tab"]["cat_lang"] . " AS d
            WHERE
                a.idart = b.idart AND
                b.idclient = '" . cSecurity::toInteger($client) . "' AND
                a.online = 1 AND
                b.idart = c.idart AND
                c.idcat = d.idcat
            GROUP BY c.idcat, online, d.startidartlang";
}

Otherwise, the check in line 484 stating if ($db->f('online') > 0) { will never be true, which makes some following clauses untrue as well if they occur in specific combinations.

muratpurc commented 1 year ago

I looked in the history of the CONTENIDO versions to see when this change was made and couldn't find anything until 4.8.3. The area around the SQL statement has not been changed for over 10 years.

Also, by adding a.online = 1 AND in the SQL statement, we wouldn't get start articles that are offline. I'm not sure if that's a good idea.

The line SUM(a.online) AS online sums the values of the online column in the result. It may be that the SUM() function returns NULL in your case because there are NULL values in the online column.

tone2tone commented 1 year ago

We had this problem before and I am quite sure I submitted something to it around January 2021. I know for sure that I had to tweak my 4.10.1 original versions to make it work for some installations. I looked into it again, and the other solution would be to remove "online" from the "group by". Obviously, the combination of "sum" and "group by" consolidates the "online" value to something incorrect.

muratpurc commented 1 year ago

I've found the related post in CONTENIDO forum, it was an issue with MySQL 8.

Can you try the following for yourself, i.e. the line

SUM(a.online) AS online,

change in

SUM(COALESCE(a.online, 0)) AS online,

This may help to correctly add the values of the a.online column.

Can you also check whether there really are numbers in the online field of the con_art_lang table?

SELECT DISTINCT(online) FROM `con_art_lang`;
tone2tone commented 1 year ago

Sorry to say that your latest suggestion doesn't help. And yes, the "online" fields are set to 1 correctly in the database (which is evident anyway as my code change suggestions lead to the correct category display colors). I really think that the aggregation of values is doubled through SUM and GROUP BY which should be one or the other, not both at the same time.

Faar400 commented 1 year ago

As much as i understood this early morning, it could be that SUM(online) and GROUP BY(online,...) is doubeled. https://www.w3resource.com/sql/aggregate-functions/sum-with-group-by.php https://learnsql.com/blog/sql-sum-group-by/ The best way, i think, would be to test these both SQL with phpmyadmin, once with GROUP BY (online,...) and once without online.

Faar400 commented 1 year ago

I let run both the 1st SQL once with group by online und once without group by online and got the same result. So this is my SQL i've testet: SELECT c.idcat AS idcat, SUM(a.online) AS online, d.startidartlang FROM con_art_langAS a, con_artAS b, con_cat_artAS c, con_cat_langAS d WHERE a.idlang = '1' AND a.idart = b.idart AND b.idclient = '1' AND a.online = '1' AND b.idart = c.idart AND c.idcat = d.idcat GROUP BY c.idcat, d.startidartlang; The problem with this code view is, that both, editor and SQL, uses the ` sign. So this editor doesn't know which is code an which is code formatting.

        It seems, group by online is not neccessary.
        But MySQL 8.x may mention missing signs ` and ' as an error.
        Perhaps there is a need to add all the ` and ' signs.            
Faar400 commented 1 year ago

OK, putting correct SQL code into the <> formatter does not work properly.

muratpurc commented 1 year ago

It is a very common practice to use the SUM() function with the GROUP BY clause, to calculate the sum for each group.

I don't see a wrong syntax in the generated SQL statement, but it seems like that we have some other points to talk about.

1. The online column in the GROUP BY clause The column online was added to the GROUP BY clause in order fix the issue with the ONLY_FULL_GROUP_BY mode. You'll get a MySQL error, if this mode is enabled and the column is missing. But, adding this column online changes the result set of the query, which still shouldn't be a problem, since the while-loop afterwards should be able to deal with it.

I wonder if it would work for you @tone2tone , if you remove the column online from the GROUP BY clause and make sure that the ONLY_FULL_GROUP_BY mode is not enabled.

2. The value of variable $syncoptions The variable $syncoptions is used to control the synchronization of articles from one language to another one. In my opinion, the variable should have the value -1 when you call up the article overview without doing anything with the synchronization. However, the variable $syncoptions has the value 0 when you regularly access the article overview. This results in a SQL statement (see condition if ($syncoptions == -1) {) in which the language ID does not appear in the WHERE clause, meaning that the query covers all the client's languages. This is wrong in my opinion, because the article overview is meant to be displayed for the current language.

But once you send the "Synchronize from" form from the action area (top left frame above article overview) by selecting the value "-- None --", then the variable $syncoptions will have the value -1 which results in generation of the correct SQL statement.

@tone2tone : Can you please check, if it works for you when you send the "Synchronize from" form with no selected value in the dropdown.

tone2tone commented 1 year ago

Hi Murat, there is no need to test anything with syncfunctions, as the database values for all articles and categories are correct. In most setups, the original query doesn't seem to produce problems, yet in others, it does. By changing the query in deleting "online" from the "group by" section, the problem is definetely solved, based on the very same database values as before. And I beg to differ when you say: It is a very common practice to use the SUM() function with the GROUP BY clause, to calculate the sum for each group. That's not really the case or let's say you seem to have a misconception here how these two are mixed. It is common to GROUP BY over ONE value and then SUM some other value based on this group, not twice on the same value. It makes no sense to GROUP something before counting. The most common case it something like: GROUP BY "cars" (other than bicycles or motorcycles) and then sum up all "doors = 5" to find all cars who have 5 doors.

tone2tone commented 11 months ago

I seem to have found one trigger that causes a difference in behaviour regarding this issue. Now that HostEurope upgraded to MySQL 8.0, websites with the original contenido code are beginning to show the "wrong" coloured icons whilst versions in MySQL5.7 didn't and would only work correct with the adapted code versions suggested above.

muratpurc commented 11 months ago

Did I understand correctly that the current version of CONTENIDO works correctly under MySQL 8.0 and under MySQL 5.7 only if you remove the online field from the GROUP BY clause?

tone2tone commented 11 months ago

Not quite, but almost. I can confirm that it works correctly under MySQL 8.0 by removing the "online" field from the "GROUP BY" clause (and it won't work if the online-field remains there). Under 5.7 I can't tell anymore; it remember that the error strangely occured on SOME installations only. At that time, HostEurope had SOME selected hosting packages running on 8.0, but the majority still on 5.7. In retrospect, I'd say that the 5.7 versions could deal with the original source code (but the 5.7 do also work when taking the "GROUP BY online" out), whilst the 8.0 don't . Not related to that, as I pointed out earlier, the "GROUP BY online" makes no sense in conjunction with the SUM-function. It would only work if ADDITIONALLY the "a.online = 1 AND" line would be added twice (as per my first post) which then would kind of double the SUM-functionality which is unnecessary. You don't need to group a value to sum the very same value up.