@carlitorweb Unfortunately you have made the moving of the file and the changes inside the moved file with the same one commit, so GitHub does not show a moved and changed file but a deleted and a new file, so the history got lost. To avoid that, moving a file and changing its content have to be made with 2 separate commits. It’s not a big problem, but please remember that for future PR‘s.

@carlitorweb Unfortunately you have made the moving of the file and the changes inside the moved file with the same one commit, so GitHub does not show a moved and changed file but a deleted and a new file, so the history got lost. To avoid that, moving a file and changing its content have to be made with 2 separate commits. It’s not a big problem, but please remember that for future PR‘s.

Copy that. Really not notice this when doing the changes, sorry. If is neccessary I can do all again, not problem.

If is neccessary I can do all again, not problem.

@carlitorweb Not necessary. All ok.

@richard67 sometimes, even when you rename a file and do a lot of changes, git is not able to detect the rename.

@carlitorweb thank you very much, now we need the two tests to get it merged.

@laoneo you are welcome, glad to help. I will continue with the rest.

I don't know how "hungry" $db = $this->getDatabase(); is, but as far as I see $db is only used once. Maybe it's more efficient to move this->getDatabase down to the place where it's used (if it's used)?

                case 'random':
                    $articlesModel->setState('list.ordering', $this->getDatabase()->getQuery(true)->rand());
                    break;

Would it be possible to write also a system test for this module, similar as we did in #40092? If you need help how to get started with system tests, you can find some information here.

Can you also fix the code style issues?

@laoneo done, please check if is okay

After fix the others similar PRs I did, I will do what you mention about system test

As less functionality you change as better as the module should basically get converted only to the new structure. New features should be done in a separate pr on the 5.0-dev branch.

@laoneo I added the cy test. I runned it, and it work. Let me kown if is okay like this for do it for the others PR

@laoneo I added the cy test. I runned it, and it work. Let me kown if is okay like this for do it for the others PR

Nice, thank you very much!

Look like the cache implementation in this module is completely removed in this PR ?

It doesn't make sense to use a separate cache in a module as we have caching in the core which caches all modules.

It doesn't make sense to use a separate cache in a module as we have caching in the core which caches all modules.

Just curious, even 3rd party modules are cached?

Yes, when the module has a cache option, then it caches automatically according to the global cache option https://github.com/joomla/joomla-cms/blob/4.4-dev/libraries/src/Document/Renderer/Html/ModuleRenderer.php#L83.

It doesn't make sense to use a separate cache in a module as we have caching in the core which caches all modules

@laoneo We will have to be careful with this change. This module has Mode parameter. If set to Dynamic, Id of category will be get directly from request. If we use default cache from ModuleRenderer, the cachemode is static, cache key is not aware of this dynamic Category ID change and I think it won't work as expected.

In short, we will have to test and make sure it works properly when Mode parameter of the module set to Dynamic.

Thats why I would change as less logic as possible, because this pr is about converting and not somezhing else.

But this PR is not just converting to new structure, it also changes how it works and could be dangerous.

I will close the PR. Then make a new one later changing only the structure. Apologies for the problems.

@carlitorweb Thanks for working on this. This module, unfortunately, harder to convert than others which do not use Cache.

No, you are right. The cache will not work. I just remember I made a module for a project using the cache as I was doing here getting the article ID from a request, and I needed include the article ID in the cachekey for make it work properly.

Just saw the opportunity to try to change the module a bit, and I toke it. Anyway, is experience I got for others PR - not change nothing is not necessary to change.

Just saw the opportunity to try to change the module a bit, and I toke it

Better do it in separate PR so that it is easier to review :).

I would like to add this pr. Can you not just revert th3 breaking changes and reopen?

@laoneo here, I revert the helper and also added the group option you can find in https://github.com/joomla/joomla-cms/blob/338a9402a2702fa5d30bd16acf61138558bcf18f/modules/mod_articles_category/mod_articles_category.php

There is also the cache part but I did not included because not sure if is needed with what you wrote before about we have caching in the core which caches all modules. Remember, for the cache work properly in this module, in case of the dynamic mode need to know the article ID and use it for generate the cachekey

There is also the cache part but I did not included because not sure if is needed with what you wrote before about we have caching in the core which caches all modules

Just for information, yesterday, I tested a different PR related to cache and now I understand this better. This module has owncache parameter, not cache like other modules. So on the said line of code https://github.com/joomla/joomla-cms/blob/4.4-dev/libraries/src/Document/Renderer/Html/ModuleRenderer.php#L83, $params->get('cache', 0) == 1 returns false, it is not being cached by automatically by ModuleRenderer , and implement cache logic by it own

So we could not remove cache logic in this module. The same is applied to other modules which has owncache parameter. For these modules, it's output depends on certain variables from request, so it could not use static cache implementation from ModuleRenderer

@laoneo Not sure why you merge this? The module cache feature is lost completely by this PR.

He reverted the cache code so the helper is as before. Why should something get lost now?

He reverted the cache code so the helper is as before. Why should something get lost now?

We meant this in our previous comments:

But I got confuse with your comment about the cache. But then @joomdonation comment about the cache should be included

Can you test #40316?

Can you test #40316?

I already did, is what I had wrote in case you said we needed include this