Feature Language Change Documentation Required PR-5.0-dev Pending

User tests: Successful: Unsuccessful:

avatar alikon
alikon
2 May 2023

Summary of Changes

  • add the plg_system_logrotation feature to a proper scheduled task plugin

Testing Instructions

Hint: The execution times and hours shown for tasks in the administrator are in the UTC timezone.

New installation

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.

Update

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.

Function test

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.

Actual result BEFORE applying this Pull Request

Log rotation is done with the "System - Log Rotation" plugin.

Expected result AFTER applying this Pull Request

New installation

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.

Update

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:

  • Interval days is same as the cache timeout of the old system plugin was before the update.
  • Number of logfiles to keep is the same as before for the old system plugin.

If the old "System - Log Rotation" plugin was disabled before the update, there is no task for that plugin.

Function test

Only one file "1.joomla_update.php" left, which is equal to the "joomla_update.php" before.

Link to documentations

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

avatar alikon alikon - open - 2 May 2023
avatar alikon alikon - change - 2 May 2023
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 2 May 2023
Category Unit Tests Repository Administration com_admin com_banners com_categories com_config com_contact com_content com_contenthistory com_finder com_installer
avatar joomla-cms-bot
joomla-cms-bot - comment - 2 May 2023

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.

avatar joomla-cms-bot joomla-cms-bot - change - 2 May 2023
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
avatar alikon alikon - change - 2 May 2023
Title
add2scheduler-logsdelete-logsrotate
[4] add2scheduler-logsdelete-logsrotate
avatar alikon alikon - edited - 2 May 2023
f4e3b84 2 May 2023 avatar alikon cs
avatar alikon alikon - change - 2 May 2023
The description was changed
Labels Added: Language Change PR-4.4-dev
avatar sandewt
sandewt - comment - 2 May 2023

[EDIT comment cleared]

avatar richard67
richard67 - comment - 2 May 2023

As far as I know, 4.4 will not get any new features, so this PR should be made for 5.0. @laoneo Please confirm.

avatar laoneo
laoneo - comment - 2 May 2023

Thats correct

avatar brianteeman
brianteeman - comment - 2 May 2023

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

avatar alikon
alikon - comment - 2 May 2023

As far as I know, 4.4 will not get any new features

really ? where this has been comunicated ??

avatar sandewt
sandewt - comment - 2 May 2023

[4.3] missing plugin attribute in .xml folder

Oops, mistake

avatar joomla-cms-bot joomla-cms-bot - change - 2 May 2023
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
avatar laoneo laoneo - change - 2 May 2023
Title
[4] add2scheduler-logsdelete-logsrotate
[5] add2scheduler-logsdelete-logsrotate
avatar laoneo laoneo - edited - 2 May 2023
avatar richard67 richard67 - change - 2 May 2023
Labels Added: Feature PR-5.0-dev
Removed: PR-4.4-dev
avatar richard67
richard67 - comment - 2 May 2023

@alikon I as so free to solve the merge conflict for you after the rebase. The changed files will look ok on GitHub after the next upmerge from 4.4-dev to 5.0-dev.

avatar alikon
alikon - comment - 2 May 2023

@richard67 that's the more easy part ? ?

avatar alikon
alikon - comment - 2 May 2023

@brianteeman

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

avatar joomla-cms-bot joomla-cms-bot - change - 6 May 2023
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
avatar alikon alikon - change - 6 May 2023
Labels Added: NPM Resource Changed
avatar alikon alikon - change - 14 May 2023
Labels Removed: NPM Resource Changed
avatar joomla-cms-bot joomla-cms-bot - change - 10 Jun 2023
Category Administration Language & Strings Front End Libraries Plugins Administration Language & Strings Front End Plugins
avatar richard67
richard67 - comment - 11 Jun 2023

@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.

avatar richard67
richard67 - comment - 13 Jun 2023

@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?

avatar HLeithner
HLeithner - comment - 13 Jun 2023

@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
avatar joomla-cms-bot joomla-cms-bot - change - 13 Jun 2023
Category Administration Language & Strings Front End Plugins SQL Administration com_admin Postgresql Language & Strings Installation Libraries Front End Plugins
avatar richard67
richard67 - comment - 14 Jun 2023

@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.

avatar richard67
richard67 - comment - 14 Jun 2023

For migrating the parameters of the obsolete logrotation system plugin to a new task and then uninstalling the obsolete plugin we can use the method from my PR #40768 , if that gets accepted.

avatar richard67
richard67 - comment - 15 Jun 2023

@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.

f4aabd7 17 Jun 2023 avatar alikon 2task
avatar richard67
richard67 - comment - 17 Jun 2023

@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.

avatar alikon
alikon - comment - 17 Jun 2023

do you want 2 pr ?
cause now we already have 2 different task plugin with just 1 pr

avatar richard67
richard67 - comment - 17 Jun 2023

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.

avatar richard67
richard67 - comment - 17 Jun 2023

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.

avatar richard67
richard67 - comment - 17 Jun 2023

@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.

avatar alikon
alikon - comment - 18 Jun 2023

@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

avatar richard67
richard67 - comment - 18 Jun 2023

@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.

avatar alikon
alikon - comment - 18 Jun 2023

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 ???

avatar richard67
richard67 - comment - 18 Jun 2023

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.

avatar richard67
richard67 - comment - 25 Jun 2023

@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)

avatar alikon
alikon - comment - 26 Jun 2023

I still think this PR should be divided into 2 separate PRs

now I think it too ?

avatar alikon
alikon - comment - 26 Jun 2023

i'll do 2 different pr's so set to draft

avatar alikon alikon - change - 27 Jun 2023
Title
[5] add2scheduler-logsdelete-logsrotate
[5] add2scheduler-rotatationlog
avatar alikon alikon - edited - 27 Jun 2023
avatar alikon alikon - change - 27 Jun 2023
The description was changed
avatar alikon alikon - edited - 27 Jun 2023
avatar alikon alikon - change - 27 Jun 2023
Title
[5] add2scheduler-rotatationlog
[5] add2scheduler-rotationlog
avatar alikon alikon - edited - 27 Jun 2023
avatar richard67
richard67 - comment - 2 Jul 2023

@alikon I've allowed myself to update your branch to latest 5.0-dev and fix the conflict. Please pull the changes before you continue to work on your local clone.

avatar richard67
richard67 - comment - 4 Jul 2023

@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.

avatar richard67
richard67 - comment - 4 Jul 2023

@alikon I've again allowed myself to update your branch to latest 5.0-dev and fix the conflict. Please pull the changes before you continue to work on your local clone.

avatar alikon alikon - change - 5 Jul 2023
The description was changed
avatar alikon alikon - edited - 5 Jul 2023
avatar richard67
richard67 - comment - 24 Jul 2023

@alikon Could you rename the update SQL scripts to something newer than "5.0.0-2023-07-12.sql" (= newest update SQL right now in the 5.0-dev branch), e.g. to "5.0.0-2023-07-24.sql"? Thanks in advance.

avatar richard67
richard67 - comment - 6 Aug 2023

@alikon I've again allowed myself to update your branch to latest 5.0-dev and fix the conflict which was caused by the change of the "compat" plugin from system to behavior group. Please pull the changes before you continue to work on your local clone.

avatar richard67
richard67 - comment - 25 Aug 2023

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?

avatar brianteeman
brianteeman - comment - 25 Aug 2023

I am confused I only have a rotate logs and not a delete logs

image

image

avatar richard67
richard67 - comment - 25 Aug 2023

@alikon It needs again to rename the update SQL scripts, this time to something newer than "5.0.0-2023-08-21.sql". I suggest to use "5.0.0-2023-08-22.sql" for this PR here.

avatar alikon
alikon - comment - 26 Aug 2023

@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

avatar richard67
richard67 - comment - 1 Sep 2023

@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.

Testing Instructions

New installation

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.

Update

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.

Actual result BEFORE applying this Pull Request

Log rotation is done with the "System - Log Rotation" plugin.

Expected result AFTER applying this Pull Request

New installation

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.

Update

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:

  • Interval days is same as the cache timeout of the old system plugin was before the update.
  • Number of logfiles to keep is the same as before for the old system plugin.

If the old "System - Log Rotation" plugin was disabled before the update, there is no task for that plugin.

avatar richard67
richard67 - comment - 1 Sep 2023

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.

avatar alikon alikon - change - 2 Sep 2023
The description was changed
avatar alikon alikon - edited - 2 Sep 2023
avatar richard67 richard67 - change - 2 Sep 2023
The description was changed
avatar richard67 richard67 - edited - 2 Sep 2023
avatar richard67 richard67 - change - 2 Sep 2023
The description was changed
avatar richard67 richard67 - edited - 2 Sep 2023
avatar richard67 richard67 - change - 2 Sep 2023
The description was changed
avatar richard67 richard67 - edited - 2 Sep 2023
avatar richard67
richard67 - comment - 2 Sep 2023

@heelc29 The PR is ready for testing. Code style error is not related to the PR but to the current 5.0-dev branch. Could you test this one, too? Testing instructions can be found in the description of this PR. Thanks in advance.

avatar richard67 richard67 - change - 2 Sep 2023
The description was changed
avatar richard67 richard67 - edited - 2 Sep 2023
avatar richard67 richard67 - change - 2 Sep 2023
The description was changed
avatar richard67 richard67 - edited - 2 Sep 2023
avatar richard67 richard67 - change - 2 Sep 2023
Labels Added: Documentation Required
avatar richard67
richard67 - comment - 3 Sep 2023

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.

avatar richard67 richard67 - change - 3 Sep 2023
The description was changed
avatar richard67 richard67 - edited - 3 Sep 2023
avatar richard67 richard67 - test_item - 3 Sep 2023 - Tested successfully
avatar richard67
richard67 - comment - 3 Sep 2023

I have tested this item ✅ successfully on f70dc24

Done all the tests - new install, update and function - on MySQL 8 and PostgreSQL 14.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/40519.

avatar richard67 richard67 - alter_testresult - 3 Sep 2023 - richard67: Tested successfully
avatar richard67 richard67 - test_item - 3 Sep 2023 - Tested successfully
avatar richard67
richard67 - comment - 3 Sep 2023

I have tested this item ✅ successfully on c092000

Done all the tests - new install, update and function - on MySQL 8 and PostgreSQL 14.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/40519.

avatar richard67 richard67 - alter_testresult - 3 Sep 2023 - richard67: Tested successfully
avatar HLeithner HLeithner - change - 3 Sep 2023
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2023-09-03 12:52:15
Closed_By HLeithner
avatar HLeithner HLeithner - close - 3 Sep 2023
avatar HLeithner HLeithner - merge - 3 Sep 2023
avatar HLeithner
HLeithner - comment - 3 Sep 2023

thanks

avatar alikon
alikon - comment - 4 Sep 2023

thank you all for your help

Add a Comment

Login with GitHub to post a comment