Fix errors specified by PHPStorm #375

aaron-junot · 2018-10-11T21:54:33Z

Fixes #277 and fixes #358.

This PR addresses all of the errors specified by PHPStorm.

drewish

Just got part of the way through this.

lib/recurly/base.php

drewish · 2018-10-12T14:57:17Z

lib/recurly/addon.php

@@ -18,6 +18,7 @@
 * @property string $revenue_schedule_type Optional field for setting a revenue schedule type. This will determine how revenue for the associated Plan should be recognized. When creating a Plan, if you supply an end_date and end_date available schedule types are never, evenly, at_range_start, or at_range_end.
 * @property DateTime $created_at The date and time the add-on was created.
 * @property DateTime $updated_at The date and time the add-on was last updated.
+ * @property string $plan_code Unique code to identify the plan.


Not a strong preference but do we want to sort these at all? Maybe float it up above the $add_on_code?

I wasn't trying to change them all around, but I figure if we do care about sorting them, then either alphabetically or the same order as the dev docs makes the most sense.

My only problem with matching the docs is that it could get out of date quickly if the docs change and this doesn't get updated. For that reason, I think maybe we should sort them alphabetically even if that's not the order of importance to the reader. The whole purpose of these is for the IDE to recognize these props, right? Not for humans reading the top of the file.

I think this is a decision that should be made at a higher level and not in this PR. I've personally gone back and forth on how to sort attributes and it would be nice to decide on a strategy for that globally but I think it's too big to tackle here.

drewish · 2018-10-12T14:59:18Z

lib/recurly/base.php

+   * @throws Recurly_NotFoundError
+   * @throws Recurly_ApiRateLimitError
+   * @throws Recurly_ServerError
+   * @throws Recurly_RequestError


All of these inherit from Recurly_Error should we just say we throw that? The only reason I wonder is, if we're this specific, if we add a new error we'd want to update all of these.

Yeah, that might be a better idea. PHPStorm automatically populated this list for me. I'll change this so that they all just document Recurly_Error

drewish

This is looking good to me. Few little things that we could change or ignore. I'll let @bhelx weigh in and do the final honors.

drewish · 2018-10-12T15:46:07Z

lib/recurly/invoice.php

   * Retrieve the PDF version of this invoice
+   *
+   * @param string $locale
+   * @return string


I suspect this is actually binary data but maybe string is the right name for it?

Right, in the same way that pack also returns a "string", but it's actually binary data. I could call out that it's actually a binary string if that helps for clarity

drewish · 2018-10-12T15:46:19Z

lib/recurly/invoice.php

+   * @param $invoiceNumber
+   * @param string $locale
+   * @param Recurly_Client $client The recurly client
+   * @return string


Same concern here.

drewish · 2018-10-12T15:47:25Z

lib/recurly/measured_unit.php

@@ -21,7 +27,7 @@ protected function uri() {
    if (!empty($this->_href))
      return $this->getHref();
    else
-      return Recurly_Addon::uriForMeasuredUnit($this->id);
+      return Recurly_MeasuredUnit::uriForMeasuredUnit($this->id);


Whoops, good catch.

Yeah thanks, looks like a copy-pasta mistake from me.

drewish · 2018-10-12T15:49:22Z

lib/recurly/purchase.php

-   * @param RecurlyClient Optional client for the request, useful for mocking the client
-   * @return Recurly_InvoiceCollection
+   * @param $purchase Recurly_Purchase Our purchase data.
+   * @param Recurly_Client $client Optional client for the request, useful for mocking the client


If we were looking for a message to copy pasta for all these @param Recurly_Client lines this one is a pretty good one.

Agreed, I'm going to change these out throughout the project

I made them all this message, we should be good on this particular note

drewish · 2018-10-12T15:50:49Z

lib/recurly/subscription.php

   */
  public function terminateWithoutRefund() {
    $this->terminate('none');
  }
+
+  /**
+   * @param $refundType


Would be nice to add the acceptable values here.

bhelx · 2018-10-12T16:54:39Z

lib/recurly/base.php

@@ -6,6 +6,8 @@
  protected $_type;
  protected $_client;
  protected $_links;
+  protected $_values;
+  protected $_errors;


Can you describe what issue this is meant to prevent?

Converting this class to a string assumes the presence of a $_values array

recurly-client-php/lib/recurly/base.php

Line 146 in e6a1e8e

ksort($this->_values);

Parsing XML to object assumes the presence of an $_errors array

recurly-client-php/lib/recurly/base.php

Line 300 in e6a1e8e

Recurly_Resource::__parseXmlToObject($rootNode->firstChild, $this->_errors);

To be honest, I don't think this is the very best solution. I think a possibly better solution would be to move the functions that reference $_values and $_errors into the class (Recurly_Resource) that defines their existence, but I'm concerned about breaking things. There's a bit of twisted logic and I didn't dig into it enough to determine whether there's ever a situation where something else that is not a Recurly_Resource would end up calling these functions that reference $_values and $_errors. So the idea was to declare them as existing, but it's up to the subclasses to define exactly what they are and assign them values.

bhelx

👍

bhelx · 2018-10-12T17:23:30Z

@aaron-suarez thanks for taking the time to do this. I think it's a nice improvement to the dev experience.

aaron-junot requested review from drewish and bhelx October 11, 2018 21:54

aaron-junot force-pushed the aaron-suarez/fix-phpstorm-warnings branch 2 times, most recently from b4bf2b4 to 8ef24dc Compare October 11, 2018 22:25

drewish reviewed Oct 12, 2018

View reviewed changes

aaron-junot force-pushed the aaron-suarez/fix-phpstorm-warnings branch from 8ef24dc to 41375f1 Compare October 12, 2018 15:41

drewish approved these changes Oct 12, 2018

View reviewed changes

aaron-junot force-pushed the aaron-suarez/fix-phpstorm-warnings branch from 41375f1 to 0b929b8 Compare October 12, 2018 16:11

Fix errors specified by PHPStorm

e6a1e8e

aaron-junot force-pushed the aaron-suarez/fix-phpstorm-warnings branch from 0b929b8 to e6a1e8e Compare October 12, 2018 16:39

bhelx reviewed Oct 12, 2018

View reviewed changes

bhelx approved these changes Oct 12, 2018

View reviewed changes

bhelx merged commit 1962582 into master Oct 12, 2018

bhelx deleted the aaron-suarez/fix-phpstorm-warnings branch October 12, 2018 17:22

This was referenced Oct 12, 2018

Added more intuitive way to get stubs #47

Closed

Small PHPStorm reported errors #376

Closed

aaron-junot mentioned this pull request Oct 30, 2018

Bump to version 2.10.6 / API version 2.16 #379

Merged

bhelx added the V2 V2 Client label Mar 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix errors specified by PHPStorm #375

Fix errors specified by PHPStorm #375

aaron-junot commented Oct 11, 2018 •

edited

Loading

drewish left a comment

drewish Oct 12, 2018

aaron-junot Oct 12, 2018

bhelx Oct 12, 2018

drewish Oct 12, 2018

aaron-junot Oct 12, 2018

drewish left a comment

drewish Oct 12, 2018

aaron-junot Oct 12, 2018

drewish Oct 12, 2018

drewish Oct 12, 2018

bhelx Oct 12, 2018

drewish Oct 12, 2018

aaron-junot Oct 12, 2018

aaron-junot Oct 12, 2018

drewish Oct 12, 2018

bhelx Oct 12, 2018

aaron-junot Oct 12, 2018

bhelx left a comment

bhelx commented Oct 12, 2018

Fix errors specified by PHPStorm #375

Fix errors specified by PHPStorm #375

Conversation

aaron-junot commented Oct 11, 2018 • edited Loading

drewish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhelx left a comment

Choose a reason for hiding this comment

bhelx commented Oct 12, 2018

aaron-junot commented Oct 11, 2018 •

edited

Loading