Allow NULL default values in @Plugin annotations

Yes, we need to be consistent across PHP <=> YAML <=> Annotation. While NULL is supported by PHP and YAML we need it also in annotations.

BTW, what is the reason for not supporting NULLs?

And this is not a "feature request", it's a bug.

I just tested NULL values in ImageItem plugin and it work. Looked also in Doctrine and they seems to handle NULLs.

I would close this as "works as designed". What you think @Xano?

However, both things are indeed by design, and are a part of Doctrine\Annotations and not Drupal anyway, so this issue may well be a won't fix. However, I'd still like to hear opinions from other Drupal developers.

I cannot imagine that this is a real problem, if you need something like that use '' or FALSE. Php is not strictly typed anyway.

As a process I would recommend to support it in Doctrine/Annotations and if they don't accept it, just live with it.

IIRC, we filtered that out in order to keep the EntityType array cleaner, since some keys were only for each kind of entity type, before we had the ConfigEntity/ContentEntity split.
See #1967294: Add a dedicated @EntityType annotation.

So, I think we can remove that filtering, but we should add test coverage to prove it is possible.

Just skimming the test, it doesn't seem to cover the new change. It should be more clear with a NULL value, as well as ensuring 'definition' isn't retained.

+++ b/core/tests/Drupal/Tests/Component/Annotation/PluginTest.php
@@ -0,0 +1,107 @@
+  public function testConstruct() {
+    $this->sut = new Plugin($this->annotationValue);
+  }

This is considered a risky test since it has no assertions.

Why a test only for the constructor while an object is constructed in ::setUp() meaning is tested in each test. I think adding the annotation to the first test it's enough.

Added IS and Beta Evaluation.

Rerolled, removed @file from test, moved to 8.2.x.

Rerolling the patch.

Fixing failed tests.

@ridhimaabrol24, thank you for working on this to fix the failing test. I see that the change is adding the 'minimum_version' property to the migrate source plugins. Is that what we want to do?

I think this needs work to investigate the 'minimum_version' property of migrate source plugins.

Edit: s/field/source

Doing some digging.

I'm not familiar with the minimum_version property of migrate source plugins so I grepped:

$ grep -r -e minimum_version core
core/tests/Drupal/Tests/Core/Database/Driver/mysql/install/TasksTest.php:  public function testNameAndMinimumVersion(bool $is_mariadb, string $expected_name, string $expected_minimum_version): void {
core/tests/Drupal/Tests/Core/Database/Driver/mysql/install/TasksTest.php:    $minimum_versio  = $tasks->minimumVersion();
core/tests/Drupal/Tests/Core/Database/Driver/mysql/install/TasksTest.php:    $this->assertSame($expected_minimum_version, $minimum_version);
core/modules/migrate/src/Annotation/MigrateSource.php:  public $minimum_version;
core/lib/Drupal/Core/Database/Driver/mysql/Install/Tasks.php:        $this->fail(t("The MySQLnd driver version %version is less than the minimum required version. Upgrade to MySQLnd version %mysqlnd_minimum_version or up, or alternatively switch mysql drivers to libmysqlclient version %libmysqlclient_minimum_version or up.", ['%version' => $version, '%mysqlnd_minimum_version' => self::MYSQLND_MINIMUM_VERSION, '%libmysqlclient_minimum_version' => self::LIBMYSQLCLIENT_MINIMUM_VERSION]));
core/lib/Drupal/Core/Database/Driver/mysql/Install/Tasks.php:        $this->fail(t("The libmysqlclient driver version %version is less than the minimum required version. Upgrade to libmysqlclient version %libmysqlclient_minimum_version or up, or alternatively switch mysql drivers to MySQLnd version %mysqlnd_minimum_version or up.", ['%version' => $version, '%libmysqlclient_minimum_version' => self::LIBMYSQLCLIENT_MINIMUM_VERSION, '%mysqlnd_minimum_version' => self::MYSQLND_MINIMUM_VERSION]));
core/lib/Drupal/Core/Database/Install/Tasks.php:      $this->fail(t("The database server version %version is less than the minimum required version %minimum_version.", ['%version' => Database::getConnection()->version(), '%minimum_version' => $this->minimumVersion()]));

That's a bit noisy with returns related to the Database system and not a plugin property. The line of note is core/modules/migrate/src/Annotation/MigrateSource.php: public $minimum_version;

I know that checkRequirements does a schema version check so I looked at DrupalSqlBase, grepping for 'minimum_schema_version'.

$ grep -r minimum_schema_version core
core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php:          if (isset($this->pluginDefinition['minimum_schema_version']) && !$this->getModuleSchemaVersion($this->pluginDefinition['source_module']) < $this->pluginDefinition['minimum_schema_version']) {
core/modules/migrate_drupal/src/Plugin/migrate/source/DrupalSqlBase.php:            throw new RequirementsException('Required minimum schema version ' . $this->pluginDefinition['minimum_schema_version'], ['minimum_schema_version' => $this->pluginDefinition['minimum_schema_version']]);

So MigrateSource defines $minimum_version, as mixed, which is not used anywhere in migrate and DrupalSqlBase uses $minimum_schema_version. The docs for $minimum_version say that migrate drupal source plugins expect minimum_version to be an integer representing the minimum installed database schema for the module. But then DrupalSqlBase tests with $minimum_schema_version, a value that is not defined or set anywhere.

That suggest to me that DrupalSqlBase should be testing with $minimum_version not $minimum_schema_version. For me, that is a bug and DrupalSQlBase should use $minimum_version.

Now, as regarding this issue is it OK to set $minimum_version to NULL on all source plugins? The short answer it yes in migrate. The annotation set the type as mixed. So that is fine. Now, for DRupalSqlBase. It is currently isn't using $minimum_schema so that won't matter. And once DrupalSqlBase is fixed it will be using $minimum_version and an isset is done before doing any comparison with it. Again, that is fine.

Fixing coding standards errors.

Created a follow up in migration for the problem described in #42, #3151732: DrupalSqlBase::checkRequirements should test version with $minimum_version.

Tested via adding patch #43 : Remove NULL filtering from \Drupal\Component\Annotation\Plugin to allow NULL in annotation default values

Changes are visible and this can be moved to RTBC

@tanubansal, thank you for interest in moving this patch along and contributing to Drupal. Please read the issue to see if screenshots are needed before adding them. Screenshots are typically only for issues that alter the UI.

As I mentioned in #3159849: Add missing tests of filepath to FileTest Comment #5 there is a wealth of information on Drupal about contributing.

@quietone : thanks ... i will take care in future

@tanubansal, thank you! If I can be of any help I can be in #migration on Drupal Slack.

Needed a reroll

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Didn't see a test patch so running locally

d6/FieldDiscoveryTest

Failed asserting that two arrays are identical.

d7/FieldDiscoveryTest

Failed asserting that two arrays are identical.

PluginTest actually passes. (not sure an issue)

Reviewing the fix

-    // Only keep actual default values by ignoring NULL values.
-    $defaults = array_filter($reflection->getDefaultProperties(), function ($value) {
-      return $value !== NULL;
-    });
+    $defaults = $reflection->getDefaultProperties();
+    unset($defaults['definition']);

Appears to be doing what the IS states.

Looks good to me.

PluginTest failing without the fix is a blocker IMO. The other two are test tweaks needed to adapt to this change, but the unit test for this code should absolutely cover this.