Encourage using DOMDocument factory methods to prevent objects broken state

[fluffycondor]

Manually created DOMNode objects have broken internal state because they don't have ownerDocument until added to one.
It leads to DOMException/warning Invalid State Error when calling ownerDocument field.
This PR encourages using factory methods that immediately bind freshly created object to ownerDocument and thus its internal state is valid.

[fluffycondor]

Looks like the failed check is unrelated to the PR

[weirdan]

I'm not so sure about this. How people use it is usually ...->appendChild(new DOM...), and that's perfectly valid.

Source: https://github.com/search?q=%22new+DOMText%22+language%3APHP&type=code

[weirdan]

Perhaps we could do something like this instead: https://psalm.dev/r/26c873ea00

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/26c873ea00

<?php

/**
 * @template _Owner as never|Doc|null
 */
class Node {
    /** @var _Owner */
    public Doc|null $owner;
    /** @psalm-assert self<never> $this */
    public function __construct() {}
}

/** @template-extends Node<null> */
class Doc extends Node { 
    /** @psalm-assert Node<Doc> $child */
    public function add(Node $child): void {}
}

$d = new Doc;
$d->owner;
/** @psalm-trace $d->owner */;
$n = new Node;
$n->__construct(); // we need to apply constructor assertions automatically
$n->owner;
/** @psalm-trace $n->owner */;
$d->add($n);
$n->owner;
/** @psalm-trace $n->owner */;

Psalm output (using commit bf57d59):

INFO: Trace - 21:30 - $d->owner: null

ERROR: DirectConstructorCall - 23:1 - Constructors should not be called directly

INFO: Trace - 25:30 - $n->owner: never

INFO: Trace - 28:30 - $n->owner: Doc

[weirdan]

Or this: https://psalm.dev/r/4bc562b64e

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/4bc562b64e

<?php

/**
 * @template _Owner as never|Doc|null
 */
class Node {
    /** @var _Owner */
    public Doc|null $owner;
    /** @psalm-self-out self<never> */
    public function __construct() {}
}

/** @template-extends Node<null> */
class Doc extends Node { 
    /** @psalm-assert Node<Doc> $child */
    public function add(Node $child): void {}
}

$d = new Doc;
$d->owner; // null
/** @psalm-trace $d->owner */;
$n = new Node;
/** @psalm-suppress DirectConstructorCall */
$n->__construct(); // we need to apply constructor assertions automatically
$n->owner; // never
/** @psalm-trace $n->owner */;
$d->add($n);
$n->owner; // Doc
/** @psalm-trace $n->owner */;

Psalm output (using commit bf57d59):

INFO: Trace - 21:30 - $d->owner: null

INFO: Trace - 26:30 - $n->owner: never

INFO: Trace - 29:30 - $n->owner: Doc

[fluffycondor]

Yes, I agree this proposal is questionable. Our company's codebase also have new DOMText(...), all of them easy to refactor though.
I agree that generics would work better here, but not only the issue with constructor assertions makes it futile now, but also broken variadic arguments support (or maybe I wrote this assertion in a wrong way?):
https://psalm.dev/r/f596da8a0d

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/f596da8a0d

<?php

/**
 * @template _Owner as never|Doc|null
 */
class Node {
    /** @var _Owner */
    public Doc|null $owner;
    /** @psalm-self-out self<never> */
    public function __construct() {}
}

/** @template-extends Node<null> */
class Doc extends Node {     
    /** @psalm-assert Node<Doc> ...$child */
    public function append(Node ...$child): void {}
}

$d = new Doc;
$d->owner; // null
/** @psalm-trace $d->owner */;
$n = new Node;
/** @psalm-suppress DirectConstructorCall */
$n->__construct(); // we need to apply constructor assertions automatically
$n->owner; // never
/** @psalm-trace $n->owner */;
$d->append($n);
$n->owner; // it's still never, not Doc :(
/** @psalm-trace $n->owner */;

Psalm output (using commit bf57d59):

INFO: Trace - 21:30 - $d->owner: null

INFO: Trace - 26:30 - $n->owner: never

INFO: Trace - 29:30 - $n->owner: never