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

PHP7.4 - Utilize 7.4 and clean ups #6288

Closed
wants to merge 1 commit into from
Closed

PHP7.4 - Utilize 7.4 and clean ups #6288

wants to merge 1 commit into from

Conversation

SpacePossum
Copy link
Contributor

No description provided.

* @var string
*/
private $classRegex = '/^\\\\?[a-zA-Z_\\x7f-\\xff](?:\\\\?[a-zA-Z0-9_\\x7f-\\xff]+)*$/';
private string $classRegex = '/^\\\\?[a-zA-Z_\\x7f-\\xff](?:\\\\?[a-zA-Z0-9_\\x7f-\\xff]+)*$/';
Copy link
Member

Choose a reason for hiding this comment

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

omg, diff is insane.
did you automated those changes? maybe we should have a rule for it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've done these manually, but I think there is a (very) risky rule do this 😅 , will take a look 👍

Copy link
Member

Choose a reason for hiding this comment

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

maybe plan it as separated ticket , and we pick it one day, if prioritised

also, don't hesitate to split PR next time to smaller pieces, like "adding types" or "dropping dedicated providers for 74", now it looks like all in one place, scary to review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe plan it as separated ticket , and we pick it one day, if prioritised

👍 adding more types is not high on my list for now TBH

split PR next time to smaller pieces

It is a split of from #6280 , but we can split into smaller bits if it is of value. However most changes are in the unittest so will get tested anyways by the CI.

Copy link
Member

Choose a reason for hiding this comment

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

let's then give time to review this PR and not rush with merge, pls :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok 👍 I'll try to split off the PR in some smaller batches, however I don't want to loose momentum here as the reviews capacity lately seems pretty limited even on the smaller PR ones, so lets keep focus on the proposed changes and not the size of the diff/PR

@coveralls
Copy link

coveralls commented Feb 15, 2022

Coverage Status

Coverage increased (+0.02%) to 93.169% when pulling b719ca8 on SpacePossum:master_74 into ae42466 on FriendsOfPHP:master.

@SpacePossum SpacePossum deleted the master_74 branch February 16, 2022 07:02
SpacePossum added a commit that referenced this pull request Feb 16, 2022
This PR was merged into the master branch.

Discussion
----------

PHP7.4 - clean up tests

part of #6288

Commits
-------

07d8374 PHP7.4 - clean up tests
SpacePossum added a commit that referenced this pull request Feb 16, 2022
This PR was merged into the master branch.

Discussion
----------

PHP7.4 - remove run time checks

part of #6288

Commits
-------

d46dd35 PHP7.4 - remove run time checks
SpacePossum added a commit that referenced this pull request Feb 16, 2022
This PR was merged into the master branch.

Discussion
----------

PHP7.4 - properties types

part of #6288

Commits
-------

6474f90 PHP7.4 - properties types
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.

None yet

3 participants