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

Fix the @var type hint for various NodeList<Node> usages. #726

Merged
merged 1 commit into from
Sep 16, 2020

Conversation

zimzat
Copy link
Contributor

@zimzat zimzat commented Sep 1, 2020

  • Remove = [] from a couple of usages, as this is now considered invalid typing.
  • Adjust a few boolean checks to explicitly check for === null now
    • (the default when not set during manual construction prior to typed properties).

Discussion:

With regard to = [] and/or |null/ = null, what approach would you want the library to take?

If you want the library to start using public NodeList $interfaces, after PHP 7.4 becomes the minimum version, then attempting to access it directly would cause uninitialized errors unless it's public ?NodeList $interfaces = null. Ensuring it's always initialized with the expected value would allow for fluid object access. At initial glance I don't see anywhere things are explicitly set to null (though I could be very wrong) so maintaining nullability doesn't seem necessary?

Direction:

  • Find a way to ensure they're always initialized as NodeList during construction?
    • Extending the __construct() method to do something like $this->interfaces = $this->interfaces ?? new NodeList([]) is one possibility.
  • Mark all of them as |null and default to = null?
  • Other?

I'd lean toward ensuring they're initialized as NodeList, but could see going some other way depending on where you see the architecture going.

Thanks!

- Remove ` = []` from a couple of usages, as this is now considered invalid typing.
- Adjust a few boolean validations to explicitly check for ` === null` now (the default when not set during manual construction).
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 86.288% when pulling 7fecf69 on zimzat:update-type-var-declarations into 4f34309 on webonyx:master.

Copy link
Collaborator

@simPod simPod left a comment

Choose a reason for hiding this comment

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

About the initialization, here's some context #675

@spawnia
Copy link
Collaborator

spawnia commented Sep 11, 2020

Remove = [] from a couple of usages, as this is now considered invalid typing.

Nicely done!

With regard to = [] and/or |null/ = null, what approach would you want the library to take?

Some properties can legimately not exist in some nodes, in which case defaulting initialization to null seems fine to me.

For properties that must not be null but may not always be initialized after construction, we should avoid typing them as nullable. We can use isset() to safely check if it is initialized (or null):

class Foo {
    var $bar;
}

$foo = new Foo();
var_dump(isset($foo->bar));
// bool(false)

$foo->bar = null;
var_dump(isset($foo->bar));
// bool(false)

This plays nicely with PHP 7.4 typed properties, which I would like to use at some point - probably far into the future.

I'd lean toward ensuring they're initialized as NodeList

That comes with a performance cost, as object construction is not free.

@vladar vladar merged commit 0cb57ca into webonyx:master Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants