braintree / braintree_php

Braintree PHP library
https://developer.paypal.com/braintree/docs/start/overview
MIT License
547 stars 224 forks source link

Property annotations are gone #310

Open slepic opened 2 years ago

slepic commented 2 years ago

General information

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 commented 2 years ago

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 https://github.com/braintree/braintree_php/commit/e35c2a2d74a10e9cec5932506486e4b8bccfa4ba

Wirone commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

for internal tracking, ticket 1994

exussum12 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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