Composer Dependency Changed PR-5.1-dev Pending

User tests: Successful: Unsuccessful:

avatar Hackwar
Hackwar
4 Jan 2024

Summary of Changes

This updates php-cs-fixer and php_codesniffer to the latest versions respectively. I'd like to do this now since that removes a deprecated package.

Testing Instructions

Everything should behave like before.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

avatar Hackwar Hackwar - open - 4 Jan 2024
avatar Hackwar Hackwar - change - 4 Jan 2024
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 4 Jan 2024
Category External Library Composer Change
avatar richard67
richard67 - comment - 4 Jan 2024

@Hackwar The composer.lock shows much more being updated than only what the description of this PR says. Is that the result of a plain composer update statement? This means you update all dependencies. Or have you explicitly only made a composer update friendsofphp/php-cs-fixer?

avatar Hackwar
Hackwar - comment - 4 Jan 2024

I did a composer update friendsofphp/php-cs-fixer --with-all-dependencies, since otherwise it doesn't update. That is also the reason why this is aimed at 5.1-dev and not 5.0-dev.

avatar Hackwar Hackwar - change - 5 Jan 2024
Labels Added: Composer Dependency Changed PR-5.1-dev
avatar joomla-cms-bot joomla-cms-bot - change - 5 Jan 2024
Category External Library Composer Change Administration com_admin com_categories com_finder com_installer com_joomlaupdate com_media com_menus com_users com_workflow Repository Front End com_ajax External Library Composer Change
avatar Hackwar
Hackwar - comment - 5 Jan 2024

I merged the necessary codestyle changes into this PR as well. No need to keep 2 PRs open for this...

avatar richard67
richard67 - comment - 5 Jan 2024

@Hackwar @brianteeman 's comment in the other, closed PR is still valid for this PR here, and I agree with him: #42604 (comment) .

For "elseif" or "case" statements, the comments are before the corresponding "elseif" or case". The CS fixer now intends them by 4 more spaces because it assumes that these comments are referring to the actual "if" or "elseif" or "case" and not the one starting in the line below.

E.g. for "elseif" in file administrator/components/com_categories/src/Model/CategoriesModel.php in line 267 and 268:

            // Case: Using only the by level filter
        } elseif ($level) {

Or for the "case" in file administrator/components/com_finder/src/Indexer/Adapter.php in lines 929 and 930:

                // Archived items should only show up when option is enabled
            case 2:
                if ($this->params->get('search_archived', 1) == 0) {

It would be better readable if we would move these comments to the line below where they belong to, e.g. for "elseif" in file administrator/components/com_categories/src/Model/CategoriesModel.php:

        } elseif ($level) {
            // Case: Using only the by level filter

Or for the "case" in file administrator/components/com_finder/src/Indexer/Adapter.php:

            case 2:
                // Archived items should only show up when option is enabled
                if ($this->params->get('search_archived', 1) == 0) {

That's just a suggestion, I would not insist on it, and I know it requires some manual work.

But I think the better readability would make it be worth the work.

avatar LadySolveig
LadySolveig - comment - 13 Jan 2024

PR phpcs fix

PR php-cs-fixer configuration update - deprecated rule
I can also do a completely separate PR for this, but I actually think it's part of the update.

avatar Quy Quy - test_item - 15 Jan 2024 - Tested successfully
avatar Quy
Quy - comment - 15 Jan 2024

I have tested this item ✅ successfully on 9760312


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

avatar laoneo
laoneo - comment - 17 Jan 2024

Just a not here, I did always do the composer update and then composer bump. Like that are the versions in the composer.json also updated.
As CS Fixer released 3.47.1 yesterday, you can update it already or we can do it then in beta.

avatar LadySolveig
LadySolveig - comment - 18 Jan 2024

Thanks @laoneo for the new version hint for CS Fixer - updated.

avatar brianteeman
brianteeman - comment - 18 Jan 2024

Not addressing #42603 (comment) ??

avatar richard67
richard67 - comment - 18 Jan 2024

Not addressing #42603 (comment) ??

It was partly addressed. Some "case" statements have been fixed, but none of the "elseifs" or "else".

avatar richard67
richard67 - comment - 18 Jan 2024

@LadySolveig Since your last commit, this PR has conflicts. And the issue which I had mentioned with my big comment has been addressed only partly, see my previous comment.

avatar LadySolveig
LadySolveig - comment - 18 Jan 2024

@richard it is not very clear to me why you did not address this when @Razzo1987 contacted you directly when he has done the manually clean up here 620d3e0

could you please clean up the the remaining comments for the elseif and else yourself @Hackwar

avatar richard67
richard67 - comment - 18 Jan 2024

@richard it is not very clear to me why you did not address this when @Razzo1987 contacted you directly when he has done the manually clean up here 620d3e0

@LadySolveig Well I saw it was dealing only with the “case” statements so I assumed the other things will be done later. My first comment was very clear, I think.

avatar brianteeman
brianteeman - comment - 18 Jan 2024

Unfortunately doing it directly without a pr (not good practice) has already been partially undone by subsequent pull requests eg #42667

avatar LadySolveig
LadySolveig - comment - 19 Jan 2024

I am not sure if I have understood you correctly, but I resolved the conflicts yesterday and made sure that the fixes for the comments remain as @Razzo1987 has already fixed them for us. @brianteeman have I overlooked something here somewhere?
Sorry, I see what you mean, that was my mistake that I pushed it directly

@richard67 I had the impression that we agreed here in the discussion with the maintainers that it would be easier to fix this directly here in the PR. But if the opinion has changed here I will merge and we will fix the rest later as you suggested in your comment.

avatar richard67
richard67 - comment - 20 Jan 2024

@LadySolveig Sorry in case if I’ve caused some confusion. Yes, it would be easier to fix it now, but we can do that also later. I don’t insist in my suggestions and am ok with merging the PR.

avatar Hackwar
Hackwar - comment - 5 Feb 2024

In the meantime, both codestyle packages have released new versions...

avatar LadySolveig LadySolveig - change - 24 Feb 2024
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2024-02-24 09:59:27
Closed_By LadySolveig
avatar LadySolveig LadySolveig - close - 24 Feb 2024
avatar LadySolveig LadySolveig - merge - 24 Feb 2024
avatar LadySolveig
LadySolveig - comment - 24 Feb 2024

Thank you !

Add a Comment

Login with GitHub to post a comment