User tests: Successful: Unsuccessful:
Hint: The execution times and hours shown for tasks in the administrator are in the UTC timezone.
Make a new installation with this PR applied.
Check if there is an enabled task scheduler plugin "Task - Rotate Logs".
Check if there is an enabled scheduled task "RotateLogs" using that plugin.
Check that the new task shall run every 30 days at the hour when the Joomla installation was made.
On a Joomla 4.4-dev version or 4.4.0 alpha 4, note the endabled status and the configuration parameters of the "System - Log Rotation" plugin.
Now either disable that plugin or enable it and optionally change some of the configuration parameters to a value different to the default.
Update to the patched package or custom update URL created by Drone for this PR.
Check if the "System - Log Rotation" plugin has been uninstalled.
Check if there is a task scheduler plugin "Task - Rotate Logs".
Check enabled status and configuration parameters of that plugin.
Check if there is a scheduled task "RotateLogs" using that plugin. If so, check the configuration parameters, too.
Repeat the previous steps with different endabled status and configuration parameters of the "System - Log Rotation" plugin.
This test can be done after the installation test or after the update test, or both.
Set the number of logfiles to keep to one in the "RotateLogs" scheduled task's parameters.
Create files "joomla_update.php", "1.joomla_update.php" and "2.joomla_update.php" with any content in the "administrator/logs" folder.
Use the "Run Test" button of the "RotateLogs" scheduled task.
Log rotation is done with the "System - Log Rotation" plugin.
The "Task - Rotate Logs" plugin is enabled, and a scheduled task "RotateLogs" is enabled to run every 30 days at the hour of the Joomla installation, and one log file shall be kept.
These are the same defaults as before for the "System - Log Rotation" plugin on a new installation.
The "System - Log Rotation" plugin has been uninstalled by the update.
A new task scheduler plugin "Task - Rotate Logs" has been created and is enabled.
If the old "System - Log Rotation" plugin was enabled before the update, a new scheduled task "RotateLogs" has been created and is enabled.
The configuration parameters of that task are set according to the old system plugin's parameters:
If the old "System - Log Rotation" plugin was disabled before the update, there is no task for that plugin.
Only one file "1.joomla_update.php" left, which is equal to the "joomla_update.php" before.
Please select:
Documentation link for docs.joomla.org: link will be added later
No documentation changes for docs.joomla.org needed
Pull Request link for manual.joomla.org:
No documentation changes for manual.joomla.org needed
Status | New | ⇒ | Pending |
Category | ⇒ | Unit Tests Repository Administration com_admin com_banners com_categories com_config com_contact com_content com_contenthistory com_finder com_installer |
Category | Unit Tests Repository Administration com_admin com_banners com_categories com_config com_contact com_content com_contenthistory com_finder com_installer | ⇒ | Administration Language & Strings Front End Plugins |
Title |
|
Labels |
Added:
Language Change
PR-4.4-dev
|
[EDIT comment cleared]
Thats correct
To Do
The plg_system_logrotation should be removed
The plg_system_actionlogs feature "Days to delete logs after" should be removed
Do I really need to repeat why you should not do that and what you can do instead
As far as I know, 4.4 will not get any new features
really ? where this has been comunicated ??
[4.3] missing plugin attribute in .xml folder
Oops, mistake
Category | Administration Language & Strings Front End Plugins | ⇒ | Repository Administration com_admin com_categories com_content com_finder com_modules com_newsfeeds com_tags com_users Language & Strings NPM Change JavaScript Front End com_contact Installation |
Title |
|
Labels |
Added:
Feature
PR-5.0-dev
Removed: PR-4.4-dev |
@richard67 that's the more easy part
Do I really need to repeat why you should not do that and what you can do instead
yes, most probably the 2nd part "should" be addressed ... better
Category | Administration Language & Strings Front End Repository com_admin com_categories com_content com_finder com_modules com_newsfeeds com_tags com_users NPM Change JavaScript com_contact Installation | ⇒ | Administration Language & Strings Libraries Front End Plugins |
Labels |
Added:
NPM Resource Changed
|
Labels |
Removed:
NPM Resource Changed
|
Category | Administration Language & Strings Front End Libraries Plugins | ⇒ | Administration Language & Strings Front End Plugins |
@alikon What is missing is the SQL changes in base.sql to add the new task plugin to the extensions table (if possible with reasonable default params) and an update SQL script for doing the same. Use the newest present update SQL which does an insert into the extensions table as example, and give the files the right name, e.g. "5.0.0-2023-06-11.sql". It could also make sense to use one common update SQL for this PR and the other one #40553 . The new plugin needs also to be added to the extensions helper.
@alikon Another thing is the name of the new plugin, the "deletelogs" part. I'd prefer it to be "logrotation" because in the log rotation functionality also covers deleting the outdated files, while a log deletion functionality not necessarily covers log rotation. The libraries for that purpose (logrotation and deletion) which I know from other programming languages also call it "log rotation", so that seems to be the common term for both together. But that's just my opinion, maybe there are other opinions. @HLeithner What do you think?
@alikon Another thing is the name of the new plugin, the "deletelogs" part. I'd prefer it to be "logrotation" because in the log rotation functionality also covers deleting the outdated files, while a log deletion functionality not necessarily covers log rotation. The libraries for that purpose (logrotation and deletion) which I know from other programming languages also call it "log rotation", so that seems to be the common term for both together. But that's just my opinion, maybe there are other opinions. @HLeithner What do you think?
I can quote my self on this: #40519 (review)
- I think deletelogs is not the best name for this plugin, rotatelogs would better
Category | Administration Language & Strings Front End Plugins | ⇒ | SQL Administration com_admin Postgresql Language & Strings Installation Libraries Front End Plugins |
@alikon Your last commit remove from actionlogs confuses me. Isn't the user actions log something in the database (see https://github.com/joomla/joomla-cms/pull/40519/files#diff-cbab29158eb01120450e460df745fcbe96cc1f01693255a0662aaaf00cca95b6L273-L288 ) and not in files?
If we handle that with a task plugin, would it not be better to use a separate one and not this one here?
It would also be better for conversion of parameters on update (which I am working on).
Right now, without this PR, we can have different settings for the user actions logs and the file based logs rotation / deletion.
When we make a new installation with 5.0-dev, the logrotation plugin is enabled and used the default values (30 days, one file), and the user actions log is also enabled but uses a default of zero days (i.e. don't delete logs).
I think we should have the same when we have migrated these things to task plugins, different ones so we can have the defaults like it was for new installations, and on update we can migrate the olkd parameters to new task plugins. When we use one plugin for both things we can only migrate the parameters of the old log rotation, and these would then apply also on the user actions logs, regardless what was adjusted for that before.
Of course that's my opinion, others are welcome.
@alikon As my PR #40768 just has been merged, I've allowed myself to update your branch for this PR here to latest 5.0-dev. I will prepare a draft PR for you to use the new method for migrating the settings of the old logrotation to task parameters and add the task in the SQL scripts for new installation and updates, too.
@alikon Has your last commit for making 2 separate task plugins something to do with my comment #40519 (comment) ? If so, you misunderstood me. I meant separate for file based logs and the user actions log. I would limit this pr here to replacing the logrotation plugin and do a separate or for the user actions log thing with a different task plugin.
do you want 2 pr ?
cause now we already have 2 different task plugin with just 1 pr
do you want 2 pr ?
cause now we already have 2 different task plugin with just 1 pr
But 2 different for log rotate and log delete doesn’t make sense. I meant it needs 2 different for normal logs and for user action logs because the latter is a database thing without rotation. This pr here was fine for replacing the old logrotation plugin before you added the user actions logs stuff with your last 2 commits. Now you have removed functionality for the user actions logs but I do not see any php code in your pr to replace that.
Today I am busy with other stuff but tomorrow I can continue to help.
P.S.: Currently people can have different expiration times for the log files, 30 days if using default values, and for user action logs, e.g. 7 days. That’s why I say use 1 plugin to replace the logrotation plugin and another one for the user actions log deletion. Then people still can have different execution cycles, and the old settings can be easily migrated, for which I will care.
@alikon Other idea: You make one task plugin for all, log file rotate and delete and action logs delete, but people can chose in a task if that task shall handle only the files or only the action logs or both, and so they can use one common task for both or 2 separate tasks with different cycles or only one but not the other. That would mean you revert the last commit for 2 plugins and then just add the necessary options to the task form.
@richard67
i'm quite lost
you can already have different schedule for different task or not at all with this pr
what is still missing is the "update path" for what i've made a 1st attempt at #40788
@richard67 i'm quite lost you can already have different schedule for different task or not at all with this pr what is still missing is the "update path" for what i've made a 1st attempt at #40788
Currently your code here is at least confusing regarding the names of your 2 plugins because the deletelogs is not deleting log files but user action logs.
Either rename the "deletelogs" so it's more clear that it is not related to the other plugin, e.g. name it "deleteactionlogs" or "deleteuseractionlogs".
Or change back so you have only 1 plugin for both and make it possible to configure in each task what it does, so you can have either one common task which does both, log file rotation and deletion and user action logs deletion, or you can have 2 different tasks for files and user action logs.
I am ok with both ways, but like it is now it is confusing.
so what it should be the name ???
"Currently your code here is at least confusing regarding the names of your 2 plugins because the deletelogs is not deleting log files but user action logs."
deleteactionlogs ???
so what it should be the name ???
"Currently your code here is at least confusing regarding the names of your 2 plugins because the deletelogs is not deleting log files but user action logs."
deleteactionlogs ???
Would be ok for me.
@alikon Besides my review comments, I still think this PR should be divided into 2 separate PRs, one for the "plg_task_rotatelogs" and another one for the "plg_task_deleteactionlogs". This would also make testing easier.
The migration of the "plg_task_rotatelogs" still would be called in the "uninstallExtensions" method in script.php like you have it now in this PR.
But the migration of the parameters for the "plg_task_deleteactionlogs" should be called in the "postflight" method like I suggested it for your other PR for the "'plg_task_privacyconsent", not in the "migrateLogRotationPlugin" function like you have it now in this PR.
Because the log rotation is by default enabled on a new installation, it would also need to add an insert statement for the task in file "extensions.sql" after the create table #__scheduler_tasks
, like I suggested with point 2 here for your other PR: #40788 (comment)
I still think this PR should be divided into 2 separate PRs
now I think it too
i'll do 2 different pr's so set to draft
Title |
|
Title |
|
@alikon As on a new installation the log rotation is switched on, we should add a new task for new installations in the same way as you did in PR #40788 for the update notification. You should use the same asset ID as in that other PR. This will of course cause a merge conflict for the one of the 2 PRs which will be merged as last, but that's the way it goes. If necessary I will help with solving that.
Thanks for this PR.
I have some comments:
* I think `deletelogs` is not the best name for this plugin, `rotatelogs` would better * Reason for this is that, delete is only a special variant of rotating logs (keep 0 copies) * Does it need it's own task for deleting logs? can't we simply say rotate 0? * maybe some descriptions could be usefull what "Maximum Logs" means for example * Please remove the system plugin what is doing the same
@HLeithner As far as I can see, all these changes have been implemented meanwhile. Could you approve the code changes or dismiss your review so GitHub doesn't show "changes requested" anymore? Or should I dismiss your review?
@brianteeman when i've started this pr it was only 1 plg for rotate + delete now there are 2 separate plugin, a leftover
@richard67 done
@alikon I've allowed myself to update the branch, fix a PHPCS error from a previous commit, rename the update SQL script again and delete the language files of the old system plugins in the sources. I will continue with the other 4 PRs.
Regarding the update SQL scripts of this and the other 4 PRs I suggest that we use the same file for all 5 PRs. This will cause a conflict for the other 4 PRs as soon as one of them gets merged, but I will be available and help to solve them.
Finally I think ti needs better testing instructions. I suggest something as follows for this PR here, and will later make suggestions also for the other 4 PRs. Feel free to just copy it and paste into the descriptions of the PRs, or modify them as you want.
Make a new installation with this PR applied.
Check if there is an enabled task scheduler plugin "Task - Rotate Logs".
Check if there is an enabled scheduled task "RotateLogs" using that plugin.
Check that the new task shall run every 30 days at the hour when the Joomla installation was made.
On a Joomla 4.4-dev version or 4.4.0 alpha 4, note the endabled status and the configuration parameters of the "System - Log Rotation" plugin.
Now either disable that plugin or enable it and optionally change some of the configuration parameters to a value different to the default.
Update to the patched package or custom update URL created by Drone for this PR.
Check if the "System - Log Rotation" plugin has been uninstalled.
Check if there is a task scheduler plugin "Task - Rotate Logs".
Check enabled status and configuration parameters of that plugin.
Check if there is a scheduled task "RotateLogs" using that plugin. If so, check the configuration parameters, too.
Repeat the previous steps with different endabled status and configuration parameters of the "System - Log Rotation" plugin.
Log rotation is done with the "System - Log Rotation" plugin.
The "Task - Rotate Logs" plugin is enabled, and a scheduled task "RotateLogs" is enabled to run every 30 days at the hour of the Joomla installation, and one log file shall be kept.
These are the same defaults as before for the "System - Log Rotation" plugin on a new installation.
The "System - Log Rotation" plugin has been uninstalled by the update.
A new task scheduler plugin "Task - Rotate Logs" has been created and is enabled.
If the old "System - Log Rotation" plugin was enabled before the update, a new scheduled task "RotateLogs" has been created and is enabled.
The configuration parameters of that task are set according to the old system plugin's parameters:
If the old "System - Log Rotation" plugin was disabled before the update, there is no task for that plugin.
Something's wrong with PostgreSQL. System tests are failing constantly. Not sure yet if related to this PR. I have to do a real test tomorrow.
Fixed. As usual it was the next value for the ID sequence of the assets table which was not increased, so when a system tests does something which also tries to create an asset, it fails due to already existing ID.
Labels |
Added:
Documentation Required
|
I've just started testing. Installation was ok. But when using the "Rin Test" button on the new task to see if it runs, I get:
PHP Warning: Attempt to read property "logstokeep" on array in /home/richard/lamp/public_html/joomla-cms-5.0-dev/plugins/task/rotatelogs/src/Extension/RotateLogs.php on line 85
Update: The reason is that the task params are an empty json in the extensions.sql, so at the very first run the logstokeep is not set. It would work, but we should not have such warning on a clean, new installation, so I have just provided a fix.
I have tested this item ✅ successfully on f70dc24
Done all the tests - new install, update and function - on MySQL 8 and PostgreSQL 14.
I have tested this item ✅ successfully on c092000
Done all the tests - new install, update and function - on MySQL 8 and PostgreSQL 14.
Status | Pending | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2023-09-03 12:52:15 |
Closed_By | ⇒ | HLeithner |
thanks
thank you all for your help
Please add more information to your issue. Without test instructions and/or any description we will close this issue within 4 weeks. Thanks.
This is an automated message from the J!Tracker Application.