Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update friendsofphp/php-cs-fixer requirement from ^2.19 to ^3.40 #1520

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

dependabot[bot]
Copy link
Contributor

@dependabot dependabot bot commented on behalf of github Dec 1, 2023

Updates the requirements on friendsofphp/php-cs-fixer to permit the latest version.

Release notes

Sourced from friendsofphp/php-cs-fixer's releases.

v3.40.0 I ate three cookies 🍪

What's Changed

Full Changelog: PHP-CS-Fixer/PHP-CS-Fixer@v3.39.1...v3.40.0

Changelog

Sourced from friendsofphp/php-cs-fixer's changelog.

Changelog for v3.40.0

  • chore: officially support PHP 8.3 (#7466)
  • chore: update deps (#7471)
  • CI: add --no-update while dropping non-compat facile-it/paraunit (#7470)
  • CI: automate --ignore-platform-req=PHP (#7467)
  • CI: bump actions/github-script to v7 (#7468)
  • CI: move humbug/box out of dev-tools/composer.json (#7472)

Changelog for v3.39.1

  • DX: introduce SwitchAnalyzer (#7456)
  • fix: NoExtraBlankLinesFixer - do not remove blank line after ? : throw (#7457)
  • fix: OrderedInterfacesFixer - do not comment out interface (#7464)
  • test: Improve ExplicitIndirectVariableFixerTest (#7451)

Changelog for v3.39.0

  • chore: Add support for Symfony 7 (#7453)
  • chore: IntegrationTest - move support of php< requirement to main Integration classes (#7448)
  • CI: drop Symfony ^7 incompat exceptions of php-coveralls and cli-executor (#7455)
  • CI: early compatibility checks with Symfony 7 (#7431)
  • docs: drop list.rst and code behind it (#7436)
  • docs: remove Gitter mentions (#7441)
  • DX: Ability to run Fixer on PHP8.3 for development (#7449)
  • DX: describe command - for rules, list also sets that are including them (#7419)
  • DX: Docker clean up (#7450)
  • DX: more usage of spaceship operator (#7438)
  • DX: Put Preg's last error message in exception message (#7443)
  • feat: Introduce @PHP83Migration ruleset and PHP 8.3 integration test (#7439)
  • test: Improve AbstractIntegrationTestCase description (#7452)

Changelog for v3.38.2

  • docs: fix 'Could not lex literal_block as "php". Highlighting skipped.' (#7433)
  • docs: small unification between FixerDocumentGenerator and ListDocumentGenerator (#7435)
  • docs: unify ../ <> ./../ (#7434)

Changelog for v3.38.1

  • chore: ListSetsCommand::execute - add missing return type (#7432)
  • chore: PHPStan - add counter to dataProvider exception, so we do not increase the tech debt on it (#7425)
  • CI: Use actions/checkout v4 (#7423)
  • fix: ClassAttributesSeparationFixer - handle Disjunctive Normal Form types parentheses (#7428)
  • fix: Remove all variable names in @var callable signature (#7429)

... (truncated)

Commits

You can trigger a rebase of this PR by commenting @dependabot rebase.


Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

  • @dependabot rebase will rebase this PR
  • @dependabot recreate will recreate this PR, overwriting any edits that have been made to it
  • @dependabot merge will merge this PR after your CI passes on it
  • @dependabot squash and merge will squash and merge this PR after your CI passes on it
  • @dependabot cancel merge will cancel a previously requested merge and block automerging
  • @dependabot reopen will reopen this PR if it is closed
  • @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
  • @dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
  • @dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
  • @dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)

Note
Automatic rebases have been disabled on this pull request as it has been open for over 30 days.

@dependabot dependabot bot added dependencies Pull requests that update a dependency file php Pull requests that update Php code labels Dec 1, 2023
Copy link

codecov bot commented Dec 1, 2023

Codecov Report

Attention: Patch coverage is 99.23664% with 1 line in your changes missing coverage. Please review.

Project coverage is 97.22%. Comparing base (c327c0f) to head (e6b625c).
Report is 1 commits behind head on master.

Current head e6b625c differs from pull request most recent head 161ceac

Please upload reports for the commit 161ceac to get more accurate results.

Files Patch % Lines
lib/CalDAV/Plugin.php 75.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1520      +/-   ##
============================================
- Coverage     97.24%   97.22%   -0.02%     
  Complexity     2834     2834              
============================================
  Files           175      175              
  Lines          8843     9018     +175     
============================================
+ Hits           8599     8768     +169     
- Misses          244      250       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@phil-davis
Copy link
Contributor

phil-davis commented Dec 2, 2023

Updated so that it uses php-cs-fixer major version 3 (no v2 any more). version 3 supports PHP 7.4 and 8, so that works fine.

php-cs-fixer v3 does lots of thing like:
Add public to const
Remove "useless" PHPdoc comments that say the same thing as the code
Adjust logical multi-line statements to put the "and/or" at the beginning of continuation lines
Adjust various indent

It changed lots of stuff, which I had to push in the 2nd commit.

We dropped older PHP in master branch, so dependabot correctly noticed that we can move to php-cs-fixer v3 in master. That is good, because we want to start adding more types, phpstan doc etc. for master to move forward to be a next major release "some day".

@phil-davis phil-davis changed the title Update friendsofphp/php-cs-fixer requirement from ^2.19 to ^2.19 || ^3.0 Update friendsofphp/php-cs-fixer requirement from ^2.19 to ^3.40 Dec 2, 2023
@phil-davis phil-davis self-assigned this Dec 3, 2023
@phil-davis
Copy link
Contributor

@DeepDiver1975 @staabm I don't think that you need to do "too long" review of this, because is it just "what php-cs-fixer" does. But I would appreciate your feedback to say that you think it is reasonable to do.

@phil-davis phil-davis force-pushed the dependabot/composer/friendsofphp/php-cs-fixer-tw-2.19or-tw-3.0 branch 2 times, most recently from 4c7ca99 to 9757809 Compare December 4, 2023 02:21
@phil-davis phil-davis force-pushed the dependabot/composer/friendsofphp/php-cs-fixer-tw-2.19or-tw-3.0 branch from 9757809 to 5fbe1da Compare December 11, 2023 12:43
@@ -159,7 +148,6 @@ public function getMultipleCalendarObjects($calendarId, array $uris);
* calendar-data. If the result of a subsequent GET to this object is not
* the exact same as this request body, you should omit the ETag.
*
* @param mixed $calendarId

Choose a reason for hiding this comment

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

I like having explicit docs about mixed types. it allowes the api user to differentiate where we accidentally forgot to define a type and where we explicitly declared mixed.

maybe this is a rule we could skip?

Copy link
Contributor

@phil-davis phil-davis Dec 11, 2023

Choose a reason for hiding this comment

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

maybe this is a rule we could skip?

agree - explicitly writing "mixed" means that someone should have thought about it and that mixed is really allowed.

https://cs.symfony.com/doc/rules/phpdoc/no_superfluous_phpdoc_tags.html

I pushed a commit to allow mixed PHPdoc tags.

but what do we do about existing places where mixed is in the PHPdoc?
For example, I doubt that $calendarId can be anything. It should have a better PHPdoc clue than mixed
So the existing places that have mixed in PHPdoc (and were auto-removed in this PR) maybe do not actually allow the full set of mixed as inputs.

Choose a reason for hiding this comment

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

For example, I doubt that $calendarId can be anything. It should have a better PHPdoc clue than mixed

I assume we have releases with such phpdocs - which means I am fine with it.

separate PRs can be submitted to improve/narrow these places if possible.

@phil-davis
Copy link
Contributor

@staabm please have a "quick" look. Let me know if you are OK with moving the code forward like cs-fixer suggests.

* @var string
*/
protected $senderEmail;

/**
* Creates the email handler.
*
* @param string $senderEmail. The 'senderEmail' is the email that shows up

Choose a reason for hiding this comment

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

we lost a param type with this change

Copy link
Contributor

Choose a reason for hiding this comment

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

fixed. I think that the "." on the end of the varibale name was what confused cs-fixer.
And I accidentally removed the whole line!

Copy link
Member

Choose a reason for hiding this comment

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

comment still missing ....

lib/CalDAV/Xml/Notification/SystemStatus.php Show resolved Hide resolved
@clxmstaab
Copy link

(reviewed with the wrong account ;-))

Copy link
Contributor Author

dependabot bot commented on behalf of github Jan 1, 2024

A newer version of friendsofphp/php-cs-fixer exists, but since this PR has been edited by someone other than Dependabot I haven't updated it. You'll get a PR for the updated version as normal once this PR is merged.

@phil-davis phil-davis force-pushed the dependabot/composer/friendsofphp/php-cs-fixer-tw-2.19or-tw-3.0 branch from e6b625c to 5909f2e Compare June 4, 2024 08:52
@phil-davis
Copy link
Contributor

Rebased and applied changes suggested by php-cs-fixer 3.58.1
I will have a look at the diffs now!

Copy link
Member

@DeepDiver1975 DeepDiver1975 left a comment

Choose a reason for hiding this comment

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

Did a quick scan and left a comment or two ....

@@ -111,10 +111,10 @@
$stmt = $pdo->prepare('UPDATE calendarobjects SET uid = ? WHERE id = ?');
$counter = 0;

while ($row = $result->fetch(\PDO::FETCH_ASSOC)) {
while ($row = $result->fetch(PDO::FETCH_ASSOC)) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is the leading \ removed? Is the speed benefit on lookup no longer of interest? 😕

Choose a reason for hiding this comment

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

There is no speed benefit. This script runs in the global namespace, so there's no point in prefixing anything with a \.

@@ -256,18 +255,18 @@ protected function validateTextMatch($check, array $textMatch)
* This is all based on the rules specified in rfc4791, which are quite
* complex.
*
* @param DateTime $start
* @param DateTime $end
* @param \DateTime $start
Copy link
Member

Choose a reason for hiding this comment

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

.... and here \ is added .....

* @var string
*/
protected $senderEmail;

/**
* Creates the email handler.
*
* @param string $senderEmail. The 'senderEmail' is the email that shows up
Copy link
Member

Choose a reason for hiding this comment

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

comment still missing ....

@staabm
Copy link
Member

staabm commented Jun 5, 2024

Rebased and applied changes suggested by php-cs-fixer 3.58.1
I will have a look at the diffs now!

just a random idea while looking at the PR. if its not too much work, it might make sense to separate the test/ changes from the rest (to ease review)

@DeepDiver1975
Copy link
Member

how do we move on? @phil-davis @clxmstaab @staabm - THX

@staabm
Copy link
Member

staabm commented Sep 13, 2024

I am fine with moving ahead/merge. there is no value in arguing about CS.

@phil-davis any strong opinions?

@derrabus
Copy link

The @param mixed and @return mixed removals are actually bad and should be reverted. CS Fixer can be configured to allow them.

@phil-davis phil-davis force-pushed the dependabot/composer/friendsofphp/php-cs-fixer-tw-2.19or-tw-3.0 branch from 161ceac to 5615c3f Compare November 5, 2024 05:41
@phil-davis phil-davis marked this pull request as draft November 5, 2024 05:53
@phil-davis
Copy link
Contributor

I will look through this again to see what state it is in, and address various comments above. So I have set to Draft for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file php Pull requests that update Php code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants