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

Property annotations are gone #310

Open
slepic opened this issue Jan 7, 2022 · 8 comments
Open

Property annotations are gone #310

slepic opened this issue Jan 7, 2022 · 8 comments
Labels

Comments

@slepic
Copy link

slepic commented Jan 7, 2022

General information

  • SDK/Library version: >=6.5.0
  • Environment: General
  • Language, language version, and OS: PHP, any

Issue description

In 6.5.0, property annotations were removed from "entity" classes. I need those back, our application heavily relies on static analysis and this is absolute breaker or we have to ignore SA on those places or we can bake in our own annotations that may go out of sync easily. This really shoudn't have came out in a minor release. It doesnt seem like a mistake. More like a decision. But still, I don't understand the reasoning for the removal tbh...

@slt
Copy link
Contributor

slt commented Mar 7, 2022

I just came along to second this issue - it seems really strange that they were stripped out?

We use phpstan for static analysis, and without these property annotations, all of these magic properties are not documented, even just reading the code it is hard to tell what exists and what data types they are?

Output from phpstan:
`


Line src/.................................php


102 Access to an undefined property Braintree\Transaction::$status.
103 Access to an undefined property Braintree\Transaction::$id.


`

Seems to have been stripped out in this commit e35c2a2

@Wirone
Copy link

Wirone commented Sep 7, 2022

We use this library in GetResponse and we also rely on static analysis. We wanted to upgrade to newest version, but 6.5.0 introduces unreasonable change, described in this issue 😩 I understand that from maintainers point of view it's easier to point users to docs instead, but from developers' perspective it's a massive deterioration in terms of code quality and type safety, not to mention development speed (lack of code completion).

Really, introduce static analysis for this library and see what it's like 🙂

Please, consider reverting phpDocs.

@hollabaq86 hollabaq86 pinned this issue Sep 28, 2022
@hollabaq86
Copy link
Contributor

hollabaq86 commented Sep 28, 2022

Hey folks, apologies for the delay in a response from us.

TLDR: we hear you, we understand (now 🙃) that static analysis via phpstan and phpdoc property annotations are important to the community, and we're thinking of updates we can make to balance the needs of this community with our team's maintenance burdens. No ETA yet, but stay tuned.

The Longer Explanation
First of all, thank you everyone for creating issues and detailing the pains that our changes to phpdoc notation created for you. They really do help us understand and learn the needs of the developer community we serve.

I want to reference my comment when I closed issue #272 so that folks have full context:

I wanted to detail some of those decisions, here, so folks have the most context on our changes. This issue was originally created because @Property entries were out-dated. We decided to include semantic links to our public developer docs that (should) have complete lists of attributes, instead of using @Property notation. We believe this is the better solve long-term for the following reasons:

  • We utilize and reinforce that our docs are the one source of truth
  • Less lines of code to maintain in this SDK
  • Less maintenance burden ensuring @Property's are consistently up-to-date
  • Easier for our engineering teams to add features without having to know about phpDoc @Property (most of our developers work in Ruby)

In addition to the above, please understand that our team of SDK engineers are on individual learning curves for each of the six Braintree SDK languages and their related community preferences that we're tasked with supporting. We sometimes encounter issues where we are aware of a resource that we understand to be a standard for developers (in this case, phpdoc), but are unaware of widely used tools that use this standard (phpstan). We understand this is a very painful example of this very scenario 😭

Our reference docs should always be considered the source of truth as an API reference. I can't go into specifics as to why, but please trust me on this. That said, we understand y'all need to be able to use phpstan without it yelling about all these missing property types and we will find a compromise!

I don't have an ETA on when or how phpdoc annotations will be updated, as we need to think through how to introduce support for @Property annotations without creating another maintenance gap down the road. I can say that we will find a compromise, and I imagine that will include our own internal use of phpstan 🙂.

We'll post comments here in this issue when we have more concrete updates. In the meantime, thank you for your patience.

@hollabaq86
Copy link
Contributor

for internal tracking, ticket 1994

@exussum12
Copy link

Our reference docs should always be considered the source of truth as an API reference.

Are they in a format which can at least be used as an ide helper ? or even better a project of stubs ?

see https://github.com/nunomaduro/larastan/tree/master/stubs

These can be used in a similar way, though i'm still unsure why its as hard as it sounds to add them back into the file?

@exussum12
Copy link

I have made a repo which works with phpstan.

https://github.com/SykesCottages/phpstan-braintree

You should be able to
composer require sykescottages/phpstan-braintree
and have phpstan coverage. Issues welcome on the repo!

@slepic
Copy link
Author

slepic commented Nov 2, 2022

@exussum12 cool
we have something like this directly inside the project that uses it, but its only for the classes/props we use - so nowhere near the completeness of yours... however I'm probably not gonna switch to yours and instead wait what the maintainers of this package decide to do..

@exussum12
Copy link

I agree that ideally this wouldn't exist or that Braintree would host it. This issue has been open for close to a year so was just hoping to bridge gaps until Braintree support something offfically.

When that happens I will add a conflict into the json (like there is now for earlier versions) so this can't be installed along with a supported solution

hollabaq86 pushed a commit that referenced this issue Jan 11, 2023
* set to php 7.2 in sdk docker image to get tests running

* Add unit tests for RiskData

* drop RiskDataScore integration check

* update README

* Require PHP >=7.3.0 (#309)

* Update README.md

* Remove AmexExpressCheckout

* Remove deviceSessionID and fraudMerchantID from lib/

* Update CreditCardTest integration

* Remove deviceSessionID and fraudMerchantID from rest of tests

* Remove masterpass (#311)

* Remove MasterpassCard, MasterpassCardDetails, and relevant tests

* Address remaining NEXT_MAJOR_VERSION notes (#314)
* Rename isUsingInstanceProxy to isUsingProxy
* Rename isAuthenticatedInstanceProxy to isAuthenticatedProxy
* Remove TRANSACTION_EXTERNAL_VAULT_CARD_TYPE_IS_INVALID  error code
* Remove snake case parameters

* Rename Android Pay to Google Pay (#310)

* Php 8 support (#308)

* update docker image to php8.0

* bump phpunit to >=9.0

* setUp and teardown must be void compatible

* rename CloseTagTest, fix breaking changes from phpunit7-> 9 in unit tests

* address phpunit deprecation warnings

* make integration tests setUp void compatible

* fix breaking changes in integration tests

* update tests to resolve deprecation warnings

* Don't create phpunit cache file

* correct phpunit semver in composer.json

* add phpunit result cache to gitignore just in case

* update Dockerfile for php8

* Audit sanity language use

* Rename isUsingInstanceProxy to isUsingProxy

* Rename isAuthenticatedInstanceProxy to isAuthenticatedProxy

* Remove TRANSACTION_EXTERNAL_VAULT_CARD_TYPE_IS_INVALID  error code

* better name for test to check we're omitting php closing tags

* fixup deprecated tests from result of merging major update branch

* DateTimeImmutable support (public issue #278) (#316)

* Remove TRANSACTION_EXTERNAL_VAULT_CARD_TYPE_IS_INVALID  error code

* support DateTimeImmutableObjects for date params

* Add `toArray` function to Base and Instance classes (resolves #289?) (#322)

* Final tweaks - Major version 6 (#323)

Co-authored-by: Samantha Cannillo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants