Document.php on line 261
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Legoktm
	Dec 5 2022, 5:17 PM

Description

When you run phplint on 32-bit systems, you get:

PHP Fatal error:  Cannot use float as default value for parameter $whatToShow of type int in /usr/share/mediawiki/vendor/wikimedia/idle-dom/src/Stub/Document.php on line 261

The code in question is:

	/**
	 * @param Node $root
	 * @param int $whatToShow
	 * @param NodeFilter|callable|null $filter
	 * @return NodeIterator
	 */
	public function createNodeIterator( /* Node */ $root, int $whatToShow = 0xFFFFFFFF, /* ?mixed */ $filter = null ) {
		throw self::_unimplemented();
	}

Because that's too big to fit in a 32-bit integer, it turns into a float, which breaks the type hint. This was caught by the Debian package's autopkgtest CI system.

(Can't find a project for idle-dom, so I hope Parsoid isn't too far off)

Details

Subject	Repo	Branch	Lines +/-
Upgrading wikimedia/parsoid (v0.16.0 => v0.16.1)	mediawiki/core	REL1_39	+2 -1
Upgrading wikimedia/parsoid (v0.16.0 => v0.16.1)	mediawiki/vendor	REL1_39	+158 -46 K
Wrap `unsigned long` values in the interface as described in interface spec	mediawiki/libs/IDLeDOM	main	+60 -26
Hack in a conversion for unsigned long default value	mediawiki/libs/IDLeDOM	main	+12 -5

Customize query in gerrit

Related Objects

Mentioned In: rMLID3f45896034d4: Wrap `unsigned long` values in the interface as described in interface spec
Mentioned Here: rGPARc381f80816d0: Remove Dodo from composer.json

Event Timeline

Legoktm created this task.Dec 5 2022, 5:17 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 5 2022, 5:17 PM

Can we just use PHP_INT_MAX ?

That code seems to be generated from spec/DOM.webidl which hardcodes the default:

307   // NodeFilter.SHOW_ALL = 0xFFFFFFFF
308   [NewObject] NodeIterator createNodeIterator(Node root, optional unsigned long whatToShow = 0xFFFFFFFF, optional NodeFilter? filter = null);
309   [NewObject] TreeWalker createTreeWalker(Node root, optional unsigned long whatToShow = 0xFFFFFFFF, optional NodeFilter? filter = null);

The unsigned long type is defined at https://webidl.spec.whatwg.org/#idl-unsigned-long "The unsigned long type is an unsigned integer type that has values in the range [0, 4294967295]."

To support 32-bit platforms I think that means it needs to be a PHP float, or use gmp/bcmath. Doesn't look like https://wiki.php.net/rfc/64bit-integer-type went anywhere.

Would be nice to know how much this function is used in gauging the severity of this, or whether the error can be safely ignored.

https://codesearch-beta.wmcloud.org/bundled/?q=%28createTreeWalker%7CcreateNodeIterator%29&files=.php suggests that it's unused so we could get away with ignoring lint errors for now?

The following text is in WebIDL.md in the IDLeDOM repo. (I must have written it, but don't remember doing so!)

Note that while the WebIDL `unsigned long` type is unsigned, with a range
of [0, 4294967295], the PHP `int` type is signed, and on 32-bit
platforms may have a range of [−2147483648, 2147483647].  To encode an
WebIDL `unsigned long` type in a PHP int, the following steps are
followed *on all platforms regardless of the value of `PHP_INT_MAX` on
that platform*:

1. Let `x` be the WebIDL `unsigned long` value to encode.
2. If `x` < 2147483648, return a PHP `int` whose value is `x`.
3. Otherwise `x` ≥ 2147483648. Return a PHP int whose value is `x − 4294967296`.

Note that this is the same as casting to a 32-bit int in most languages.

To decode an WebIDL `unsigned long` value from a PHP `int`, the following
steps must be followed:

1. Let `x` be the PHP int value to decode.
2. If `x` ≥ 0, return an WebIDL `unsigned long` whose value is `x`.
3. Otherwise `x` < 0. Return an WebIDL `unsigned long` whose value is `x + 4294967296`.

Note that in PHP this is the same as performing a bit-wise AND of the
`int` value with the long constant `0xffffffff`.

This implies that it's the code that emits the literal parameter default value from the IDL as 0xFFFFFFFF which is at fault, and that this should be emitted as -1 instead.

That said, since the code is unused, the priority is definitely low. I think there are some PHP 8+ compatibility fixes in the queue, though, I should get those released while I'm at it.

Change 865769 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/libs/IDLeDOM@main] Hack in a conversion for unsigned long default value

https://gerrit.wikimedia.org/r/865769

Change 865771 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/libs/IDLeDOM@main] Wrap `unsigned int` values in the interface as described in interface spec

https://gerrit.wikimedia.org/r/865771

Change 865769 abandoned by Arlolra:

[mediawiki/libs/IDLeDOM@main] Hack in a conversion for unsigned long default value

Reason:

In favour of Icf31304350c6a22fde07c68b4604303e0738e6db

https://gerrit.wikimedia.org/r/865769

Change 865771 merged by jenkins-bot:

[mediawiki/libs/IDLeDOM@main] Wrap `unsigned long` values in the interface as described in interface spec

https://gerrit.wikimedia.org/r/865771

cscott mentioned this in rMLID3f45896034d4: Wrap `unsigned long` values in the interface as described in interface spec.Dec 9 2022, 6:43 PM

Maintenance_bot removed a project: Patch-For-Review.Dec 9 2022, 7:30 PM

I've cherry-picked the fix into Debian for now, thanks!

For fixing it properly in 1.39, it seems we need to update the version in Dodo, tag a new Dodo release, update the Dodo version in Parsoid, and then tag a new Parsoid release and bump all in mediawiki/vendor.

Or, we could cherry-pick c381f80816d09674815aed94be3f6665a271e33c into REL1_39 of Parsoid, bump the IDLeDOM version there, and just do the Parsoid release and update in mediawiki/vendor, and drop Dodo entirely. Any reason not to go with that?

Legoktm assigned this task to cscott.Dec 12 2022, 1:50 AM

• JMcLeod_WMF moved this task from Needs Triage to Dodo on the Parsoid board.Dec 15 2022, 3:21 PM

• JMcLeod_WMF edited projects, added Parsoid (Dodo); removed Parsoid.

• JMcLeod_WMF added a project: Content-Transform-Team-WIP.

• JMcLeod_WMF moved this task from Backlog to In Progress on the Content-Transform-Team-WIP board.

• JMcLeod_WMF moved this task from In Progress to To Verify on the Content-Transform-Team-WIP board.Dec 19 2022, 4:12 PM

I think I'm more likely to bump package versions in the REL1_39 of Parsoid, since that usually gets done for various security/compatibility reasons in 1.39.x releases any way.

Change 869325 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/vendor@REL1_39] Upgrading wikimedia/parsoid (v0.16.0 => v0.16.1)

https://gerrit.wikimedia.org/r/869325

Change 869327 had a related patch set uploaded (by Reedy; author: Reedy):

[mediawiki/core@REL1_39] Upgrading wikimedia/parsoid (v0.16.0 => v0.16.1)

https://gerrit.wikimedia.org/r/869327

Change 869325 merged by Reedy:

[mediawiki/vendor@REL1_39] Upgrading wikimedia/parsoid (v0.16.0 => v0.16.1)

https://gerrit.wikimedia.org/r/869325

Change 869327 merged by jenkins-bot:

[mediawiki/core@REL1_39] Upgrading wikimedia/parsoid (v0.16.0 => v0.16.1)

https://gerrit.wikimedia.org/r/869327