Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
tone2tone opened this issue Sep 11, 2023 · 16 comments
Labels
bug Something isn't working

Comments

@tone2tone
Copy link
Contributor

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
Copy link
Collaborator

muratpurc commented Sep 11, 2023

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

  • The category has no start article
  • Or the category has no online article

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

muratpurc commented Sep 12, 2023

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor

Faar400 commented Sep 25, 2023

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
Copy link
Contributor

Faar400 commented Sep 25, 2023

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_lang AS 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
Copy link
Contributor

Faar400 commented Sep 25, 2023

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

@muratpurc
Copy link
Collaborator

muratpurc commented Sep 25, 2023

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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.

@muratpurc muratpurc added the bug Something isn't working label Feb 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants