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.

Sign up for GitHub

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

Jump to bottom

LCH-6253: Fix php-stan issues in content-hub-php #157

Merged
merged 18 commits into from
Dec 19, 2022
Merged

LCH-6253: Fix php-stan issues in content-hub-php #157

merged 18 commits into from
Dec 19, 2022

Conversation

narendradesai
Copy link
Contributor

saxumVermes
saxumVermes reviewed Dec 19, 2022
View reviewed changes
phpstan.neon
paths:
- ./src
ignoreErrors:
- '#Class [a-zA-Z0-9\\_:\(\).]* extends [a-zA-Z0-9\\_:@\(\).]*#'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why were these error ignored? @narendradesai

Copy link
Contributor Author

@narendradesai narendradesai Dec 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@saxumVermes This added to suppress below error:
Class Acquia\ContentHubClient\ContentHubClient extends @final class GuzzleHttp\Client

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a really good example why this should not be done. Please next time include the reason in the solution! I've just read why they tagged the Client as final. the reason being, when version 8 comes out the will set the class (not just annotating) as final. They wanted to mark it as a deprecated that way, which will break backward compatibility with all our implementations essentially disabling - without refactor - updating to an upper version.
This needs to be taken care of soon.

saxumVermes
saxumVermes requested changes Dec 19, 2022
View reviewed changes
Copy link
Contributor

@saxumVermes saxumVermes left a comment

Choose a reason for hiding this comment

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

I left 2 minor comments, but the second requires some action, otherwise looks good!

src/CDF/CDFObject.php Outdated Show resolved Hide resolved
@saxumVermes
Copy link
Contributor

Thanks to phpstan we identified that one of the failures pointed out a future, planned deprecation: GuzzelHttp/Client will be set to final. This will prevent any implementing side to extend the class. This needs two follow-up actions:

  • Create a ticket which resolves this issue to make it compatible with guzzle v8
    • We need to fix every such issue related to this deprecation and re-enable this check so future deprecations can be identified
  • Set maximum version to guzzle v7 in composer.json in this ticket

@narendradesai Please create the appropriate tickets.

saxumVermes
saxumVermes reviewed Dec 19, 2022
View reviewed changes
composer.json Outdated Show resolved Hide resolved
saxumVermes
saxumVermes approved these changes Dec 19, 2022
View reviewed changes
Copy link
Contributor

@saxumVermes saxumVermes left a comment

Choose a reason for hiding this comment

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

This ticket PR in its form is good to be merged. We'll handle the guzzle issue in a separate task.

@saxumVermes saxumVermes merged commit 818126c into 2.x Dec 19, 2022
@narendradesai narendradesai mentioned this pull request Dec 19, 2022
Closed
amangrover90 added a commit that referenced this pull request Jan 10, 2023
@saxumVermes @narendradesai @amangrover90
LCH-6300: Use composition instead of inheritance. (#159)
4507569 
* LCH-6272: Remove type hint, allow integer values for scroll. (#156)

* LCH-6253: Fix php-stan issues in content-hub-php (#157)

* LCH-6238: Enable PHP8 job.

* LCH-6238: Update phpunit version.

* LCH-6238: Add directory at the end to generate deprecation message.

* LCH-6238: Stop phpunit tests on failure.

* LCH-6238: Adding phpstan.

* LCH-6238: Adding level 1 php stan.

* LCH-6253: Adding phpstan config file.

* LCH-6253: Scanning directories.

* LCH-6253: Autoload directories.

* LCH-6253: Autoload directories.

* LCH-6253: Autoload directories.

* LCH-6253: Autoload directories.

* LCH-6253: Ignoring class not found erros.

* LCH-6253: Fixing phpstan errors.

* LCH-6253: Ignore phpstan errors.

* LCH-6253: Adding excludepaths.

* LCH-6253: Updating the return type hint and fixing guzzle version less than 7.5.

* LCH-6253: Use <8.0 as the pinned upper version.

Co-authored-by: Amandeep Singh <arshgrover32@gmail.com>
Co-authored-by: Peter Pajor <peter.pajor@acquia.com>

* LCH-6300: Use composition instead of inheritance.

* LCH-6300: Provide return types.

* LCH-6300: Update neon config.

* LCH-6300: Move implementation into common trait, use composition for logging client.

* LCH-6300: Disable coverage report.

* LCH-6300: Reenable coverage report.

* LCH-6300: Setting a minimum version for coveralls.

Co-authored-by: Narendra Shenvi Desai <shenvidesainarendra@gmail.com>
Co-authored-by: Amandeep Singh <arshgrover32@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saxumVermes saxumVermes saxumVermes approved these changes

@amangrover90 amangrover90 Awaiting requested review from amangrover90

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@narendradesai @saxumVermes @amangrover90