-
Notifications
You must be signed in to change notification settings - Fork 347
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
base: master
Are you sure you want to change the base?
Update friendsofphp/php-cs-fixer requirement from ^2.19 to ^3.40 #1520
Conversation
Codecov ReportAttention: Patch coverage is
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. |
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: 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". |
@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. |
4c7ca99
to
9757809
Compare
9757809
to
5fbe1da
Compare
@@ -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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 thanmixed
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.
@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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment still missing ....
(reviewed with the wrong account ;-)) |
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. |
e6b625c
to
5909f2e
Compare
Rebased and applied changes suggested by php-cs-fixer 3.58.1 |
There was a problem hiding this 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)) { |
There was a problem hiding this comment.
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? 😕
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment still missing ....
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) |
how do we move on? @phil-davis @clxmstaab @staabm - THX |
I am fine with moving ahead/merge. there is no value in arguing about CS. @phil-davis any strong opinions? |
The |
Updates the requirements on [friendsofphp/php-cs-fixer](https://github.com/PHP-CS-Fixer/PHP-CS-Fixer) to permit the latest version. - [Release notes](https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/releases) - [Changelog](https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/blob/master/CHANGELOG.md) - [Commits](PHP-CS-Fixer/PHP-CS-Fixer@v2.19.0...v3.40.0) --- updated-dependencies: - dependency-name: friendsofphp/php-cs-fixer dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com>
161ceac
to
5615c3f
Compare
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. |
Updates the requirements on friendsofphp/php-cs-fixer to permit the latest version.
Release notes
Sourced from friendsofphp/php-cs-fixer's releases.
Changelog
Sourced from friendsofphp/php-cs-fixer's changelog.
... (truncated)
Commits
27d2b32
prepared the 3.40.0 release45a4530
chore: officially support PHP 8.3 (#7466)d50d5b6
CI: move humbug/box out of dev-tools/composer.json (#7472)9ae580f
CI: automate --ignore-platform-req=PHP (#7467)9fbe27e
chore: update deps (#7471)0a3a684
CI: add --no-update while dropping non-compatfacile-it/paraunit
(#7470)0bc713d
CI: bump actions/github-script to v7 (#7468)7735bbf
bumped version857046d
prepared the 3.39.1 releaseffc5780
fix: OrderedInterfacesFixer - do not comment out interface (#7464)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)