User tests: Successful: Unsuccessful:
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.
Everything should behave like before.
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
Status | New | ⇒ | Pending |
Category | ⇒ | External Library Composer Change |
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.
Labels |
Added:
Composer Dependency Changed
PR-5.1-dev
|
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 |
I merged the necessary codestyle changes into this PR as well. No need to keep 2 PRs open for this...
@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.
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.
I have tested this item ✅ successfully on 9760312
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.
Not addressing #42603 (comment) ??
Not addressing #42603 (comment) ??
It was partly addressed. Some "case" statements have been fixed, but none of the "elseifs" or "else".
@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.
@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
@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.
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.
@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.
In the meantime, both codestyle packages have released new versions...
Status | Pending | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2024-02-24 09:59:27 |
Closed_By | ⇒ | LadySolveig |
Thank you !
@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 acomposer update friendsofphp/php-cs-fixer
?