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

Handle file objects like file_upload #520

Merged
merged 3 commits into from
Sep 24, 2018
Merged

Handle file objects like file_upload #520

merged 3 commits into from
Sep 24, 2018

Conversation

ob-stripe
Copy link
Contributor

r? @brandur-stripe
cc @stripe/api-libraries

Like stripe/stripe-ruby#689, but for PHP.

@brandur-stripe
Copy link
Contributor

Very fast work! :)

LGTM.

@ob-stripe ob-stripe force-pushed the ob-file-resource branch 2 times, most recently from 882d56c to 97de5f4 Compare September 22, 2018 20:10
@ob-stripe
Copy link
Contributor Author

I pushed another commit to only use files.stripe.com for file creation requests. I had to change a few things to make it possible to override the API base per request, by making it possible to pass an api_base option.

I also had to split the file creation tests into separate test classes, because they need their own setup/teardown methods.

@ob-stripe
Copy link
Contributor Author

ptal @brandur-stripe


namespace Stripe;

class FileCreationTest extends TestCase
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the nit, but given that the separate test file breaks convention elsewhere, could we add a short comment here that just explains that these tests have been moved to a separate suite because the methods they invoke use a separate API hostname?


namespace Stripe;

class FileUploadCreationTest extends TestCase
Copy link
Contributor

Choose a reason for hiding this comment

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

(And same on this one.)

@ob-stripe ob-stripe merged commit 000c46a into master Sep 24, 2018
@ob-stripe ob-stripe deleted the ob-file-resource branch September 24, 2018 19:03
@ob-stripe
Copy link
Contributor Author

Released as 6.18.0.

@ruudk
Copy link
Contributor

ruudk commented Oct 11, 2018

Next time, please release this as a new major as it's a breaking change.

@ob-stripe
Copy link
Contributor Author

@ruudk \Stripe\File is aliased to \Stripe\FileUpload, so it shouldn't be a breaking change. Can you share an example where existing code would cease to work after upgrading to 6.18.0?

@ruudk
Copy link
Contributor

ruudk commented Oct 11, 2018

First, PHPStan failed for it:


 ------ ------------------------------------------------------------------------- 

  Line   src/Platform/StripeBundle/Service/Stripe.php                             

 ------ ------------------------------------------------------------------------- 

  812    Return typehint of method                                                

         xxx\Platform\StripeBundle\Service\Stripe::uploadFil  

         e() has invalid type Stripe\FileUpload.                                  

  823    Call to static method create() on an unknown class Stripe\FileUpload.    

 ------ ------------------------------------------------------------------------- 

We fixed that, and then on production this happened:
image 2018-10-11 at 3 21 17 pm

@ob-stripe
Copy link
Contributor Author

Weird, it looks like for some reason the class aliasing isn't working in your environment, but I have no idea what could cause that -- if you can use \Stripe\File, that means that the File.php file was loaded and the class_alias statement should definitely have been executed.

@ruudk
Copy link
Contributor

ruudk commented Oct 11, 2018

I think the issue is that File.php is never loaded. The autoloader is searching for FileUpload and cannot find it. I see that you include the File.php inside init.php. But we never run include that file.

@ruudk
Copy link
Contributor

ruudk commented Oct 11, 2018

The standard is to use Composer autoloader, so no init.php file will be loaded. That means that the only way to solve this class alias is to create the file FileUpload.php and there include the class_alias function?

@ob-stripe
Copy link
Contributor Author

Ah yes, that's likely the issue. Our test suite didn't catch this even when using autoload because we have tests for both File and FileUpload, so File.php would get loaded.

I'll push a fix with what you suggested. Thanks very much for your help, and sorry for the trouble!

@ob-stripe
Copy link
Contributor Author

@ruudk I've just released 6.19.4 to fix the autoloading issue. I guess you've already updated your integration to use File instead of FileUpload so you don't need to upgrade, but it will prevent other users from running into the same issue.

Thanks again for your help in investigating and fixing this issue! Much appreciated :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants