PHP 8.2 Improvements

[swissspidy]

Fixes #532

[westonruter]

Why not continue to use $this->head and $this->body? Is there a reason to switch to using $this->properties?

[westonruter]

Just for the sake of performance?

[swissspidy]

I ran into some issues when I originally tried that, but I might have been doing something wrong. I can give it another try 👍

[swissspidy]

Just tested again. Even on PHP 8.1 I am getting lots of Undefined property warnings in this case.

Even when doing something seemingly simple like this:

// Throws warning
$this->html = $html;
return $this->html;

// Works
$this->html = $html;
return $html;

// Also works
$this->properties['html'] = $html;
return $this->properties['html'];

I also just analyzed the AmpProject\Dom\DocumentTest::testNormalizeDomStructure() test case for this.

1. Calling normalizeDomStructure() calls $this->head
2. This triggers __get()
3. The getter for head runs, and it sees that it isn't set yet, which causes a call to normalizeDomStructure() again. The property hasn't been set yet.
4. That one then calls moveInvalidHeadNodesToBody at the end
5. That one then triggers Undefined property: AmpProject\Dom\Document::$head.

No such issues happen with the current $properties approach.

[westonruter]

I think the issue at question is this note:

PHP will not call an overloaded method from within the same overloaded method. That means, for example, writing return $this->foo inside of __get() will return null and raise an E_WARNING if there is no foo property defined, rather than calling __get() a second time. However, overload methods may invoke other overload methods implicitly (such as __set() triggering __get()).

[westonruter]

In other words, if a caller is trying to access $x->foo which invokes a magic getter, the getter must not also try to access $x->foo although it can access $x->bar. So there can't be any recursive reference to the current magic property being gotten.

[westonruter]

I just realized a potential large simplification here. Why if each of the properties were instead just declared as null?

Suggested change

	/**
	* Associative array for lazily-created, cached properties for the document.
	*
	* @var array
	*/
	private $properties = [];
	public $xpath
	public $html;
	public $head;
	public $body;
	public $charset;
	public $viewport;
	public $ampElements;
	public $ampCustomStyle;
	public $ampCustomStyleByteCount;
	public $inlineStyleByteCount
	public $links;

The phpdoc notwithstanding.

[westonruter]

I see you did this in 525e918 but then reverted. Did you try private instead of public? This would model what is done with the $links property already.

[westonruter]

I see the problem with that approach. It's that private access in the class won't cause the getter to be invoked. I think I have a way around that, and that is to manually invoke __get() for methods on Document. I'm going to give it a try, though the commit may not be pushed until Monday.

[westonruter]

Bah. Doing this makes PHPStan blow up with a lot of "Access to private property" errors.