PHPC-1709: Add typing information to arginfo #1337

alcaeus · 2022-07-22T09:18:21Z

PHPC-1708
PHPC-1711
PHPC-2015
PHPC-2115

This PR adds stub files to generate arginfo. As a first proof of concept, this is done for BSON classes, but other classes will follow. Wherever we can't add return types yet (e.g. for interfaces and non-final classes where this constitutes a BC break) we declare a tentative return type to alert people to the upcoming change. In the stub files, this is characterised by the @tentative-return-type annotation in the docblock for the corresponding method.

There are a couple of requirements for this, which will be added to the contribution docs later:

arginfo headers have to be generated using build/gen_stub.php. To get this file, phpize needs to have been called with PHP 8.0 or later
Changing stub files entails regenerating arginfo files, as well as removing any .lo files for the class in question. I haven't yet managed to define the arginfo files as dependency for the respective class which would enable automatic recompilation.
Create stub files for BSON classes
Create stub files for remaining classes
Update contributing docs
~~Integrate arginfo generation into build process (if possible)~~
Add checks to build process to ensure stub files are up to date (if the above is not possible)

src/BSON/Binary.c

alcaeus · 2022-07-22T09:20:57Z

tests/bson/bson-decimal128_error-001.phpt

@@ -11,7 +11,7 @@ require_once __DIR__ . '/../utils/basic.inc';

 echo throws(function() {
    new MongoDB\BSON\Decimal128([]);
-}, MongoDB\Driver\Exception\InvalidArgumentException::class), "\n";
+}, TypeError::class), "\n";


I'm not entirely sure about these changes - some people may see this as a BC break. I'll have to double-check why the exception class changes for this and ObjectId.

Does this change apply to all PHP versions? It calls to mind some of the variations between PHP 7.x and 8.0 when we were implementing PHPC-1631.

The only other example of split tests for TypeError that comes to mind is:

https://github.com/mongodb/mongo-php-driver/blob/master/tests/session/session-startTransaction_error-004.phpt

https://github.com/mongodb/mongo-php-driver/blob/master/tests/session/session-startTransaction_error-005.phpt

It affects all PHP versions, since we unconditionally add argument types to methods. I'll take another look at this tomorrow to figure out where this is coming from.

I'm pretty sure this is the result of parameter types now being validated by arginfo instead of zpp. That's probably something we should document in release notes (you can add documentation-needed on the ticket and make a note of this in a comment). I don't think there's any place to mention this in the extension docs, though -- it'd be pedantic to add this to every method's changelog.

Yes, I believe adding this to each method's changelog would be overkill. Maybe a paragraph in the release notes would be appropriate for this.

alcaeus · 2022-07-25T07:34:01Z

@jmikola this is ready for a first look before I continue with the rest of these classes. Build failures on PHP 8.1 are due to deprecation warnings since we're introducing tentative return types for BSON interfaces. The affected classes will be updated in the next step once we've confirmed that the general direction is ok.

src/BSON/DBPointer.stub.php

src/BSON/Binary.stub.php

src/BSON/Binary_arginfo.h

src/BSON/Javascript.stub.php

src/BSON/RegexInterface.stub.php

src/BSON/UTCDateTime.stub.php

alcaeus

Added generate-function-entries to existing stubs and added stubs for exception classes. See a couple of notes below.

alcaeus · 2022-07-27T12:34:59Z

src/BSON/Binary.stub.php

+    final class Binary implements BinaryInterface, JsonSerializable, Type, \Serializable
+    {
+        /** @var int */
+        public const TYPE_GENERIC = 0x01;


I've added these in a separate commit since we used to use the libbson enum to define these constants. Since these are protected by the BC promise, I don't mind hardcoding the values here but can revert the change if you'd rather use libbson values.

Ah, I didn't consider that. Since php_phongo_binary_init_ce isn't going away, I think keeping the zend_declare_class_constant_long lines in there is sensible. This is going to come up with other files as well, and I'd rather depend on the libmongoc constants if possible, since that might also catch a bug on their end.

If not, I'd at least want to have comments in these stub files referring to the libmongoc constant we're hard-coding. For example:

public const TYPE_GENERIC = 0x01; /* BSON_SUBTYPE_BINARY */

Try (may requires 8.2 to generate the arginfo)

/** @cvalue BSON_SUBTYPE_BINARY */ public const TYPE_GENERIC = UNKNOWN;

See php/php-src@1362fef

Ah, that is nice. I was already using the 8.2 file (the previous version didn't handle constants), so I switched to using @cvalue.

alcaeus · 2022-07-27T12:36:37Z

src/BSON/Binary.c

-
-	INIT_NS_CLASS_ENTRY(ce, "MongoDB\\BSON", "Binary", php_phongo_binary_me);
-	php_phongo_binary_ce                = zend_register_internal_class(&ce);
+	php_phongo_binary_ce                = register_class_MongoDB_BSON_Binary(php_phongo_binary_interface_ce, php_phongo_json_serializable_ce, php_phongo_type_ce, zend_ce_serializable);


This function is generated by specifying generate-class-entries in the stub file. It can't take care of object handlers, but it allows us to rely on the stub files for the class structure (modifiers, extended classes, implemented interfaces, class and property definitions, etc.).

src/BSON/BinaryInterface.stub.php

jmikola · 2022-07-27T13:52:06Z

src/BSON/Binary.stub.php

+
+namespace MongoDB\BSON
+{
+    final class Binary implements BinaryInterface, JsonSerializable, Type, \Serializable


Should JsonSerializable be \JsonSerializable? It's not defined in the MongoDB\BSON namespace.

Since php_phongo_binary_init_ce includes:

#if PHP_VERSION_ID >= 80000 zend_class_implements(php_phongo_binary_ce, 1, zend_ce_stringable); #endif

Would it make sense to use #if here to define two different class declarations, or perhaps just an extra line between this and the opening { to avoid duplication? Something like:

final class Binary implements BinaryInterface, JsonSerializable, Type, \Serializable #if PHP_VERSION_ID >= 80000 , \Stringable #endif {

Yes - I'll change this to \JsonSerializable. I was confused by the php_phongo_json_serializable_ce variable 🤦

Unfortunately, the #if is completely ignored in the class header (both with and without duplication of the class definition in conditionals). We're unfortunately stuck adding this ourselves until we require PHP 8.0, at which point we can add it to the stub files.

See: https://github.com/mongodb/mongo-php-driver/blob/1.14.0/php_phongo.c#L183

We may actually want to continue using php_phongo_json_serializable_ce, since it's fetched during MINIT to avoid problems on some distributions where the JSON extension was not compiled in to PHP and instead loaded dynamically. That was definitely an issue back in the 5.x releases (see: PHPC-878), and it may still apply to 7.x+. We may need to confirm with @remicollet.

And while I'm looking at the above, I think the manual fetching of DateTimeImmutable may be obsolete. I've opened PHPC-2115 to track that.

Json is always build statically in 8.x, so this hack is still useful on 7.x

(btw, only there to avoid "undefined symbol" when load order is not the correct one for linux distribution using --enable-rtld-now)

remicollet · 2022-07-28T04:59:30Z

src/BSON/Binary.stub.php

+
+/**
+  * @generate-class-entries static
+  * @generate-function-entries static


Also nice to add @generate-legacy-arginfo 80000 to take benefit of compatibility stuff when using 8.2. Example when using SensitiveParameter attributes, see

See https://github.com/pierrejoye/php_zip/blob/master/php81/php_zip.stub.php#L91
And https://github.com/pierrejoye/php_zip/blob/master/php81/php_zip_arginfo.h#L532

Also notice that is some additional compatibility checks are missing, is still possible to add them in gen_stub.php before 8.2.0 GA

I have yet to check the legacy arginfo generation and the difference between files. Since we still support PHP 7.2, I assume we'll want to use @generate-legacy-arginfo 70000?

If you use 70200 it will also generate a foo_legady_arginfo.h without type hinting.

generate-legacy-arginfo 70200 is not supported, so we could only generate legacy arginfo files for PHP 7.0. Since those don't include any argument types at all (which we want since PHP 7.2 allows for this), we'll just do without legacy arginfo for now. We've got everything we wanted covered this way as well:

argument types (mixed and unions only on PHP 8.0+)

return types for final methods/classes

tentative return types for non-final methods/classes (PHP 8.1+, no return type info in other versions)

generate-legacy-arginfo accepts any PHP_VERISON_ID, "legacy" file is generated as soon as < 80000 ;)

BTW? I really think it should be used, at least for SensitiveParameter.
And hacks for compatibility could probably be proposed in php_src so every ext will be benefit of them.

This is how OpenSource work ;)

$ ./build/gen_stub.php In ./src/MongoDB/WriteConcern.stub.php: Legacy PHP version must be one of: "70000" (PHP 7.0), "80000" (PHP 8.0), "80100" (PHP 8.1), "80200" (PHP 8.2), "70200" provided

I'll take a look at adding a compatibility mode for PHP 7.2, but at the moment it doesn't exist. As it is, legacy arginfo isn't useful for us.

This is necessary to let the generated arginfo files compile

.github/workflows/arginfo-files.yml

CONTRIBUTING.md

config.m4

php_phongo.c

tests/manager/manager-executeQuery-005.phpt

tests/exception/commandexception-getresultdocument-001.phpt

tests/exception/bulkwriteexception-getwriteresult-001.phpt

jmikola · 2022-08-01T18:04:48Z

tests/bson/bson-decimal128_error-001.phpt

@@ -11,7 +11,7 @@ require_once __DIR__ . '/../utils/basic.inc';

 echo throws(function() {
    new MongoDB\BSON\Decimal128([]);
-}, MongoDB\Driver\Exception\InvalidArgumentException::class), "\n";
+}, TypeError::class), "\n";


I'm pretty sure this is the result of parameter types now being validated by arginfo instead of zpp. That's probably something we should document in release notes (you can add documentation-needed on the ticket and make a note of this in a comment). I don't think there's any place to mention this in the extension docs, though -- it'd be pedantic to add this to every method's changelog.

jmikola

GitHub decided to stop rendering the full PR diff, but I reviewed the commits since my last review and it looks like everything has been addressed.

andrew-demb · 2023-03-10T14:06:26Z

Adding return types to interfaces breaks BC, isn't it?

As stated in https://jira.mongodb.org/browse/PHPC-1711

Similar to PHPC-1708, except for interfaces instead of final classes. This is a BC break and can only be done in 2.0, and only after the change has been communicated to users (see PHPC-1710).

Should this change be followed with a major release?

v1.15.0 just broke our implementations of CommandSubscriber due to a lack of a void return type in implementation.
https://github.com/code-tool/jaeger-mongodb-collector/blob/0c81dccc14ca516e506c1f13c4ec8ed67592392f/src/JaegerMongoDbQueryTimeCollector.php#L42

PHP Fatal error: During inheritance of MongoDB\Driver\Monitoring\CommandSubscriber: Uncaught ErrorException: Return type of CodeTool\Jaeger\MongoDb\JaegerMongoDbQueryTimeCollector::commandStarted(MongoDB\Driver\Monitoring\CommandStartedEvent $event) should either be compatible with MongoDB\Driver\Monitoring\CommandSubscriber::commandStarted(MongoDB\Driver\Monitoring\CommandStartedEvent $event): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /.../vendor/code-tool/jaeger-mongodb-collector/src/JaegerMongoDbQueryTimeCollector.php:42
Stack trace:
#0 /.../vendor/code-tool/jaeger-mongodb-collector/src/JaegerMongoDbQueryTimeCollector.php(22): Symfony\Component\DependencyInjection\Dumper\Preloader::Symfony\Component\DependencyInjection\Dumper{closure}(8192, 'Return type of ...', '/var/www/profil...', 42)
#1 /.../vendor/composer/ClassLoader.php(571): include('/var/www/prof...

jmikola · 2023-03-11T03:04:59Z

The 1.15.0 release uses tentative return types for interfaces methods. Our reasoning at the time was that this was a BC-friendly migration path since it'd have no effect on PHP 8.0 and earlier (see: phongo_compat.h).

That said, I now realize that users already on PHP 8.1 would have encountered this issue for the first time when upgrading from driver version 1.14.x to 1.15.0. I apologize for any inconvenience this may have caused on your side.

Should this change be followed with a major release?

In hindsight, it may have been preferable punt on introducing tentative return types altogether. I don't think the change alone would be enough to warrant a bump to 2.0 (there are other larger changes planned, but not yet implemented). Tentative return types perhaps make more sense in PHP core where BC breaks regularly show up in minor releases (e.g. https://www.php.net/manual/en/migration81.incompatible.php), but not PECL extensions which follow external release schedules.

PHPC-2139 is the canonical ticket for BC-breaking typing changes for 2.0. That will be the next opportunity to replace these tentative return types with actual return types.

alcaeus self-assigned this Jul 22, 2022

alcaeus commented Jul 22, 2022

View reviewed changes

alcaeus force-pushed the feature/phpc-2015 branch 2 times, most recently from 3cf104e to 6bfe74d Compare July 25, 2022 07:28

alcaeus requested a review from jmikola July 25, 2022 07:33

jmikola reviewed Jul 25, 2022

View reviewed changes

Add missing macros for declaring arguments

553d9c5

alcaeus force-pushed the feature/phpc-2015 branch from 6bfe74d to 82f3668 Compare July 27, 2022 12:32

alcaeus commented Jul 27, 2022

View reviewed changes

alcaeus added 4 commits July 27, 2022 15:32

Add macros to define disabled constructor and __wakeup

8414fb1

Define arginfo via stubs for BSON classes

b1a67c8

Declare tentative return types in interfaces

7441e3f

Add correct return type for __set_state methods

da90d6c

alcaeus force-pushed the feature/phpc-2015 branch from 82f3668 to b47a633 Compare July 27, 2022 13:32

jmikola reviewed Jul 27, 2022

View reviewed changes

remicollet reviewed Jul 28, 2022

View reviewed changes

alcaeus force-pushed the feature/phpc-2015 branch from 57ae9f0 to 1a4e1a5 Compare July 28, 2022 06:17

Generate class entries for BSON classes

4d54c6f

alcaeus force-pushed the feature/phpc-2015 branch from 1a4e1a5 to 5bda12f Compare July 28, 2022 10:33

Declare Binary class constants in stub file

cfccd01

alcaeus force-pushed the feature/phpc-2015 branch 2 times, most recently from f6a101f to a3a6604 Compare July 29, 2022 06:33

alcaeus added 2 commits July 29, 2022 14:32

Use stubs in exception classes

770e95b

Add stubs for monitoring classes

369a6df

alcaeus force-pushed the feature/phpc-2015 branch 3 times, most recently from 9efe977 to f604e6a Compare August 1, 2022 07:19

alcaeus added 2 commits August 1, 2022 09:37

Fix wrong JsonSerializable class in BSON stubs

0743d33

Disable declaration-after-statement

6a32dfb

This is necessary to let the generated arginfo files compile

alcaeus added 6 commits August 1, 2022 09:37

Use stubs for driver classes

71644db

Fix deprecation messages in tests

b6e090d

Add missing macro

773ff9d

Fix tests relying on value injection

d3e93a7

Parse parameters in disabled constructor/wakup function

1c67059

Add note about arginfo files to contribution docs

fa78349

alcaeus force-pushed the feature/phpc-2015 branch from f604e6a to aff07b4 Compare August 1, 2022 10:23

Add GitHub action to check generated arginfo files

4833b1b

alcaeus force-pushed the feature/phpc-2015 branch from aff07b4 to d469466 Compare August 1, 2022 10:26

alcaeus changed the title ~~[WIP] PHPC-2015 Use stub files for generating arginfo~~ PHPC-1709: Add typing information to arginfo Aug 1, 2022

alcaeus requested a review from jmikola August 1, 2022 11:08

jmikola requested changes Aug 1, 2022

View reviewed changes

alcaeus added 4 commits August 2, 2022 09:14

PHPC-2115: Use DateTimeInterface in UTCDateTime constructor signature

edde54c

Don't install mongodb extension in GitHub actions workflows

767dd1a

Change indentation in stub files

d4d1afe

Use individual #if conditions for each stub method

27012ad

alcaeus force-pushed the feature/phpc-2015 branch from d469466 to 0f40aec Compare August 2, 2022 08:19

alcaeus added 4 commits August 2, 2022 10:37

Make disabled constructor/wakeup methods static

0abee61

Fix bulkWrite parameter name

c9eea94

Use %d to match property count in tests

f54546a

Ensure all classes with disabled serialisation declare __wakeup

49e526e

alcaeus force-pushed the feature/phpc-2015 branch from 0f40aec to 49e526e Compare August 2, 2022 08:38

alcaeus requested a review from jmikola August 2, 2022 08:38

alcaeus mentioned this pull request Aug 2, 2022

PHPC-2093: Key Management API #1339

Merged

jmikola approved these changes Aug 2, 2022

View reviewed changes

alcaeus merged commit 6887226 into mongodb:master Aug 3, 2022

alcaeus deleted the feature/phpc-2015 branch August 3, 2022 06:42

alcaeus mentioned this pull request Aug 3, 2022

PHPC-2015: Clean up stub files for BSON classes #1341

Merged

PHPC-1709: Add typing information to arginfo #1337

PHPC-1709: Add typing information to arginfo #1337

Uh oh!

Conversation

alcaeus commented Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alcaeus commented Jul 25, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alcaeus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmikola left a comment

Choose a reason for hiding this comment

Uh oh!

andrew-demb commented Mar 10, 2023

Uh oh!

alcaeus commented Jul 22, 2022 •

edited

Loading

jmikola Jul 25, 2022 •

edited

Loading