[CLEANUP] Fix Psalm warnings in AbstractHtmlProcessor #991
Conversation
|
|
||
| /** | ||
| * The constructor. | ||
| * | ||
| * Please use `::fromHtml` or `::fromDomDocument` instead. | ||
| */ | ||
| private function __construct() | ||
| final private function __construct() |
There was a problem hiding this comment.
Wouldn't this prevent any extending class having a constructor at all? What if it needed one to perform some sort of initialization?
I actually think it would be more maintainable if the constructor is protected. That way, if any extending class needed a constructor, it could call the base class constructor. Then, if at some point in the future there was a need for the AbstractHtmlProcessor constructor to actually do anything, it wouldn't break extending classes that needed a constructor but had been unable to call the base class constructor (noting that, unlike C++, in PHP, base class constructors are not implicitly called if a constructor is defined in an extending class).
We have already encountered a similar issue with PHPUnit removing a constructor in a new major release (see #930 - they should have retained a do-nothing constructor to avoid the change being breaking).
I know that extensions of the class are not intended to be instantiated with new, and thus their constructors (if defined) should not be public. I appreciate that the idea behind this change is to prevent an extending class declaring a public constructor. However, it prevents an extending class declaring any constructor at all (AFAIK).
There was a problem hiding this comment.
We can make it protected, but we cannot drop the final or drop the constructor altogether. If we do any of these, Psalm (rightly) complains:
ERROR: UnsafeInstantiation - src/HtmlProcessor/AbstractHtmlProcessor.php:106:21 - Cannot safely instantiate class
Pelago\Emogrifier\HtmlProcessor\AbstractHtmlProcessorwith "new static" as its constructor might change in child classes (see https://psalm.dev/229)$instance = new static();
There was a problem hiding this comment.
In that case I think we should use the @psalm-consistent-constructor annotation so that child classes can have a constructor if needed, but, if so, Psalm would enforce that it has the same parameters (i.e. no parameters).
I also think the constructor should be protected but that's probably beyond the scope of this PR.
There was a problem hiding this comment.
Okay, changed accordingly.
| protected function getXPath(): \DOMXPath | ||
| { | ||
| if (!$this->xPath instanceof \DOMXPath) { | ||
| throw new \BadMethodCallException('$this->xPath has not been initialized yet.', 1617819086); |
There was a problem hiding this comment.
Could we use self::class in the message (like for getDomDocument) rather than $this which might be a bit vague?
There was a problem hiding this comment.
Yes, we can! Changed and repushed.
| @@ -60,16 +60,16 @@ abstract class AbstractHtmlProcessor | |||
| protected $domDocument = null; | |||
|
|
|||
| /** | |||
| * @var \DOMXPath | |||
| * @var \DOMXPath|null | |||
There was a problem hiding this comment.
Could we use @var ?\DOMXPath? It's a matter of style but the 'nullable' syntax is consistent with method signatures recognized by the PHP interpreter.
There was a problem hiding this comment.
Yes, this seems to work for Psalm. Changed.
oliverklee
force-pushed
the
cleanup/psalm-abstract-html-processor
branch
2 times, most recently
from
dc60718
to
b0b0a0d
Compare
- use `?ClassName` instead of `ClassName|null` in annotations - mention class names in exception messages from `getXpath()` - make the constructor protected
oliverklee
force-pushed
the
cleanup/psalm-abstract-html-processor
branch
from
b0b0a0d
to
cc45c29
Compare
No description provided.