Skip to content

Conversation

@heelc29
Copy link
Contributor

@heelc29 heelc29 commented Nov 12, 2022

Pull Request for Issue #38754 and #38786 .

Summary of Changes

Note that not all files were converted to the PSR-12 codestyle during PR #37686 eg:

  • administrator/components/com_actionlogs/src/View/Actionlogs/HtmlView.php
  • administrator/components/com_admin/src/Service/HTML/Configuration.php
  • administrator/components/com_cache/tmpl/cache/default.php
  • administrator/components/com_media/tmpl/media/default.php
  • api/components/com_media/src/Model/MediaModel.php
  • layouts/joomla/form/field/media/accessiblemedia.php
  • libraries/src/Cache/Cache.php
  • libraries/src/Component/Router/RouterViewConfiguration.php
  • plugins/fields/media/media.php
  • plugins/privacy/actionlogs/actionlogs.php
  • plugins/system/cache/src/Extension/Cache.php
  • plugins/webservices/media/media.php

These files are therefore not checked by Drone now. This is due to an overly open exclude list (marked in list above).

Testing Instructions

  • Drone works
  • Code Review
  • Install Joomla

Actual result BEFORE applying this Pull Request

2648 files are checked by phpcs

Expected result AFTER applying this Pull Request

2743 files are checked by phpcs

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

cc @HLeithner

@richard67
Copy link
Member

@heelc29 Regarding the testing instructions: "Install Joomla" is not necessary. That is included already in the "Drone works". The API and system tests include making a new installation on diverse environments, so if these pass, installation works. But it could make sense to let people run the PHPCS tests locally in their environment with ./libraries/vendor/bin/phpcs --extensions=php -p --standard=ruleset.xml . and check that this works.

@richard67
Copy link
Member

I have tested this item ✅ successfully on 0de1eff


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

@heelc29
Copy link
Contributor Author

heelc29 commented Nov 13, 2022

@richard67

Regarding the testing instructions: "Install Joomla" is not necessary

Just copied from PR #37686 - but only a part of it 😅

But it could make sense to let people run the PHPCS tests locally in their environment

This is not always possible because I have problems with the EOL in my win environment:
image

@brianteeman
Copy link
Contributor

@heelc29 there is usually a setting in your git client that will resolve that

@richard67
Copy link
Member

@heelc29 Why have you updated your branch without any need? It resets the counter for human tests and I manually have to add the test result again in the issue tracker. That causes unnecessary work and may cause the one or other PR with 2 good test results being overlooked.

@heelc29
Copy link
Contributor Author

heelc29 commented Nov 13, 2022

@heelc29 there is usually a setting in your git client that will resolve that

@brianteeman I use GitHub Desktop with default settings ... mmmh so I can't be sure that everyone else has changed it at their git client

@heelc29
Copy link
Contributor Author

heelc29 commented Nov 13, 2022

@heelc29 Why have you updated your branch without any need? It resets the counter for human tests and I manually have to add the test result again in the issue tracker. That causes unnecessary work and may cause the one or other PR with 2 good test results being overlooked.

@richard67 I thought to make sure there are no conflicts or new code style errors from the changed ruleset.

@brianteeman
Copy link
Contributor

. mmmh so I can't be sure that everyone else has changed it at their git client

Thats irrelevant. Just follow https://docs.github.com/en/get-started/getting-started-with-git/configuring-git-to-handle-line-endings

@heelc29
Copy link
Contributor Author

heelc29 commented Jan 15, 2023

Conflicts resolved in:

  • administrator/components/com_actionlogs/tmpl/actionlogs/default.php
  • libraries/src/Cache/Cache.php
  • libraries/src/Cache/CacheController.php
  • libraries/src/Cache/CacheStorage.php
  • libraries/src/Cache/Controller/CallbackController.php
  • libraries/src/Cache/Storage/FileStorage.php
  • libraries/src/Cache/Storage/MemcachedStorage.php
  • libraries/src/Cache/Storage/RedisStorage.php
  • plugins/privacy/actionlogs/actionlogs.php
  • plugins/system/actionlogs/actionlogs.php

Copy link
Member

@HLeithner HLeithner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry that this took so long, can you change the target branch to 4.3 pleasE?

@heelc29
Copy link
Contributor Author

heelc29 commented Jan 16, 2023

sorry that this took so long, can you change the target branch to 4.3 pleasE?

@HLeithner Sure, but I will wait for the next upmerge, there are many conflicts at the moment:
[4.3] Change application input access to getInput #39029 vs. Changes the code base to short array syntax #39616
And a tricky one too in libraries/namespacemap.php which I don't want to resolve. Take care here @obuisard ;)
[4.2] Fix library autoloading #39348 vs. Add namespace support to templates #39011

@heelc29 heelc29 changed the base branch from 4.2-dev to 4.3-dev January 21, 2023 13:00
@heelc29
Copy link
Contributor Author

heelc29 commented Jan 21, 2023

sorry that this took so long, can you change the target branch to 4.3 pleasE?

@HLeithner done

@heelc29 heelc29 changed the title [4.2] Convert missing files to PSR-12 code style [4.3] Convert missing files to PSR-12 code style Jan 21, 2023
Copy link
Member

@HLeithner HLeithner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@Quy Quy added PR-4.3-dev and removed PR-4.3-dev labels Jan 21, 2023
@HLeithner HLeithner merged commit b9730d5 into joomla:4.3-dev Jan 27, 2023
@heelc29 heelc29 deleted the patch-psr12 branch March 25, 2023 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants