Hhvm strict #280
Hhvm strict #280
Conversation
All Element classes converted and tests passing, running on HHVM Fixed Validators Covered Clients
All Element classes converted and tests passing, running on HHVM Fixed Validators Covered Clients Finished Getters Treating Rules and Transformer
All Element classes converted and tests passing, running on HHVM Fixed Validators Covered Clients Finished Getters Treating Rules and Transformer
…ticles-sdk-php into hhvm_strict
…ticles-sdk-php into hhvm_strict
…ticles-sdk-php into hhvm_strict
| */ | ||
| public function toDOMElement($document = null) | ||
| public function toDOMElement(\DOMDocument $document): \DOMNode |
There was a problem hiding this comment.
$document was optional before. Should it stay this way?
I think toDOMElement(\DOMDocument $document = new \DOMDocument()) would have the same semantics.
There was a problem hiding this comment.
Technically the main issue of us rendering the elements and creating by ourselves the DOMDocument, is that we will miss the correct encoding to write content to.
I feel that keeping it optional (so we create it all the time) works well only for test purposes, but for practical usage of our lib, we should drop this being optional.
What do you think?
| */ | ||
| public function toDOMElement($document = null) | ||
| public function toDOMElement(\DOMDocument $document): \DOMNode |
There was a problem hiding this comment.
| @@ -32,14 +32,10 @@ public static function create() | |||
| * | |||
| * @param \DOMDocument $document - The document where this element will be appended (optional). | |||
| * | |||
| * @return \DOMElement | |||
There was a problem hiding this comment.
Are DOMElement and DOMNode the same?
There was a problem hiding this comment.
Technically all DOMElement is a DOMNode, but not the other way around.
While parsing a DOMDocument and navigating through the children elements, only standard tags will map to DOMElement.
| * | ||
| * @return $this | ||
| */ | ||
| public function withAttribute($attribute) |
There was a problem hiding this comment.
Has this moved somewhere else?
There was a problem hiding this comment.
This method already exists on AbstractGetter, so after the refactor you can remove it.
There was a problem hiding this comment.
As @diegoquinteiro says.
Removed for no needing to keep it here.
There was a problem hiding this comment.
Hey @everton-rosario, thank you so much for putting this port together - it is a LOT of work and it looks great!
Please look at my comments, I've scanned through all the changes and pointed out some problems.
From all the comments, the most important concerns for me are:
- Can we get the performance optimizations back to the Transformer?
- Why did we change the test case input/outputs? For example, the WP rules were changed a lot, so we're not sure we're being backwards compatible.
- Why did we come back with InteractiveInsideParagraphRule?
Let me know if you wanna discuss any point!
Thanks one more time =)
| @@ -22,29 +21,25 @@ class Client | |||
| /** | |||
| * @var Facebook The main Facebook service client. | |||
| */ | |||
| private $facebook; | |||
| private \Facebook\Facebook $facebook; | |||
There was a problem hiding this comment.
Any specific reason why we dropped use Facebook\Facebook;?
There was a problem hiding this comment.
Namespace conflicts for autoload =(
| */ | ||
| protected $pageID; | ||
| protected string $pageID; | ||
|
|
||
| /** | ||
| * @var bool|false Are we using the Instant Articles development sandbox? |
There was a problem hiding this comment.
Let's use bool only instead of bool|false, as bool contains false. We would use false if we knew the variable couldn't be true.
There was a problem hiding this comment.
yes, changing.
| $facebook = new Facebook([ | ||
| 'app_id' => $appID, | ||
| 'app_secret' => $appSecret, | ||
| 'default_access_token' => $accessToken, | ||
| 'default_graph_version' => 'v2.5' | ||
| ]); | ||
|
|
||
| return new static($facebook, $pageID, $developmentMode); | ||
| return new self($facebook, $pageID, $developmentMode); |
There was a problem hiding this comment.
I don't have a strong opinion on this particular instance, but just want to make sure you know that self will point always to Client, even if it's extended. So if:
class SubClass extends Client { ... }
SubClass::create(...); // will create a Client, not a SubClassWhere using static:
class SubClass extends Client { ... }
SubClass::create(...); // will create a SubClassThere was a problem hiding this comment.
For strict code, this is the error:
Can't use new static() for Facebook\InstantArticles\Client\Client; __construct arguments are not guaranteed to be consistent in child classes This declaration neither defines an abstract/final __construct nor uses <<__ConsistentConstruct>> attribute : src/Facebook/InstantArticles/Client/Client.php:17
| // Never try to take live if we're in development (the API would throw an error if we tried) | ||
| $published = $this->developmentMode ? false : $published; | ||
|
|
||
| // Assume default access token is set on $this->facebook | ||
| $response = $this->facebook->post($this->pageID . Client::EDGE_NAME, [ | ||
| 'html_source' => $article->render(null, $formatOutput), | ||
| 'html_source' => $article->render(), |
There was a problem hiding this comment.
If we're not using $formatOutput anymore, we should remove the parameter of the method or add a deprecation notice to the docs.
| $field = $this->developmentMode ? 'development_instant_article' : 'instant_article'; | ||
|
|
||
| $response = $this->facebook->get('?id=' . $canonicalURL . '&fields=' . $field); | ||
| $instantArticle = $response->getGraphNode()->getField($field); | ||
|
|
||
| if (!$instantArticle) { | ||
| return null; | ||
| return -1; |
There was a problem hiding this comment.
We could still return null if we changed the return type to ?int, right? Returning -1 seems a little bit obscure to me (and code would not fail fast), but if you absolutely need to use -1, please update the phpdoc comment of the method accordingly.
There was a problem hiding this comment.
Nice catch.
Fixing.
| $log->debug('rule: '.get_class($this)); | ||
| $log->debug('-------'); | ||
| } | ||
| // if ($matches_context && $matches_node) { |
There was a problem hiding this comment.
Let's get rid of all these commented lines =)
There was a problem hiding this comment.
Let's also create an issue for adding proper verbosity levels.
There was a problem hiding this comment.
Indeed: #285
| * | ||
| * @return string The full qualified name of class. | ||
| */ | ||
| public function getObjClassName(): string |
There was a problem hiding this comment.
How is that different from getClassName()? If it's a leftover, let's get rid of it.
There was a problem hiding this comment.
Unfortunately using //strict it forces to call each method properly.
so self::getClassName() and $instance->getObjClassName() are required for code =(
| */ | ||
| private $rules = []; | ||
| private array<Rule> $rules = array(); |
There was a problem hiding this comment.
Let's do a project wide search for array< to review outdated documentation.
There was a problem hiding this comment.
👍
Fixed know ones.
| @@ -97,82 +91,42 @@ public static function markAsProcessed($node) | |||
| * | |||
| * @param DOMNode $node The node to clone | |||
There was a problem hiding this comment.
Outdated docs as well, let's change it to:
/**
* @param DOMNode The node of interest
* @return bool Whether the node is processed or not
*/| @@ -231,27 +185,12 @@ public function transform($context, $node) | |||
| } | |||
| $matched = false; | |||
|
|
|||
| // Get all classes and interfaces this context extends/implements | |||
There was a problem hiding this comment.
Why are we removing the performance optimization? The algorithm is 10 times slower without it.
There was a problem hiding this comment.
The changes you reverted on addRule, getRules and getAllClassTypes were all related to this optimization.
There was a problem hiding this comment.
Here's the original perf PR for reference: #46
There was a problem hiding this comment.
This is one of the discussion points.
This was all broken(after strict migration), not well tested and documented. Got rid of it.
We can add it back with proper review.
everton-rosario
force-pushed
the
hhvm_strict
branch
from
4a9295d to
162c112
Compare
Ported lint fixes from www
Hhvm strict optimization
- Vector => vec - Set => keyset - Map => dict - array() => proper structure
Hhvm strict hackarrays
|
|
This PR