-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[4.1] Fix generate web cron key #37868
Conversation
@@ -375,7 +375,7 @@ public function generateWebcronKey(EventInterface $event): void | |||
[$context, $table] = $event->getArguments(); | |||
|
|||
if ($context !== 'com_config.component' | |||
|| ($table->name ?? '') !== 'COM_SCHEDULER') | |||
|| ($table->name ?? '') !== 'com_scheduler') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can also be moved to one line, or?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean change it to, or ?
if ($context !== 'com_config.component' || ($table->name ?? '') !== 'com_scheduler')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
Strange that this is the fix as the code has always been this way and it obviously worked previously. I wonder what changed (elsewhere) to break this. Whatever it was, if it broke this then we should treat that as a red flag that it could have broken code elsewhere. |
@brianteeman I don't have any clue about it because I haven't looked at the code of schedule tasks feature before. From what I see (by just code reading), |
@@ -374,8 +374,7 @@ public function generateWebcronKey(EventInterface $event): void | |||
/** @var Extension $table */ | |||
[$context, $table] = $event->getArguments(); | |||
|
|||
if ($context !== 'com_config.component' | |||
|| ($table->name ?? '') !== 'COM_SCHEDULER') | |||
if ($context !== 'com_config.component' || $table->name !== 'com_scheduler') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do a strtolower on this please - covers both cases with minimal overhead. If this used to work - it's safer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wilsonge We can but is it really needed? The install and update SQL, from my quick check, is just com_scheduler. Name of all of our components in extensions table is also lower case, so I don't see the need for that. Strange that this feature worked before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might depend on the collation if a string comparison in MySQl is case sensitive or not. See e.g. https://stackoverflow.com/questions/5629111/how-can-i-make-sql-case-sensitive-string-comparison-on-mysql or https://stackoverflow.com/questions/5629111/how-can-i-make-sql-case-sensitive-string-comparison-on-mysql . So for people using the Joomla standard "utf8mb4_unicode_ci" it might have worked, and for people using some collation ending with "_cs" it moght not have worked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure with my above comment, it's just an idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we just compare data which is read from database, not from an SQL query, so I think it is not related.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, silly me. The comparison happens in PHP and not in a query in SQL. Seems I was blind.
I have tested this item ✅ successfully on b09ee6b This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37868. |
|
Hmm. The patch worked for me for both 4.1 and 4.2, but for my local site, it is a new installation, not updating from old version of Joomla. @brianteeman Could you please changed that line of code to the code below, then test it again to see if it works? if ($context !== 'com_config.component' || strtolower($table->name) !== 'com_scheduler') |
I have tested this item ✅ successfully on b09ee6b This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37868. |
RTC. Thanks all ! This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/37868. |
@brianteeman It broke with your PR #37387 |
@heelc29 and now we know why - thanks |
Thx |
confirmed, it works! Thank you! |
Pull Request for Issue #37806.
Summary of Changes
This PR fixes a small typo in the code cause web cron key not being generated when Web Cron enabled
Testing Instructions
Please note that I have zero experience with web cron feature, just try to fix this issue by reading the code quickly.