Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Component: base system » plugin system

.

dawehner’s picture

This would a good improvement for DX, as it removes a potential point of failure.

Xano’s picture

Issue summary: View changes

We shouldn't just add this to the annotation class, as annotated class discovery is just one of the many available discovery mechanisms (YAML and static discovery are used as well). #2458789: Introduce PluginDefinitionInterface would be an ideal place for this, but as that's not in yet and plugin managers already contain functionality to merge in default values, maybe we should add the validation there.

dawehner’s picture

Well, the advantage of putting it onto the annotation class would be that you could throw a really helpful message to the endeveloper,
unless on the manager level where you have a level of abstraction. What about doing both?

Xano’s picture

Status: Active » Needs review
FileSize
758 bytes

We essentially provide two annotation base classes: Plugin, which supports annotation arrays and which this patch adds validation to, and AnnotationBase, which is used by child classes with both single and multiple values. The latter is a little more difficult to change because of that.

Xano queued 5: drupal_1868916_5.patch for re-testing.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhedstrom’s picture

The patch in #5makes a ton of sense to me. Anything left to be done here?

jhedstrom’s picture

Status: Needs review » Needs work

Those 2 failing tests need to be resolved. Looks like it's just a matter of updating some parameters to the exception that is thrown.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Xano’s picture

Status: Needs work » Needs review

Re-testing on 8.7.x.

Xano’s picture

Version: 8.6.x-dev » 8.7.x-dev
Xano’s picture

FileSize
3.28 KB
3.28 KB
Xano’s picture

FileSize
3.36 KB

Right....

longwave’s picture

+++ b/core/tests/Drupal/Tests/Component/Annotation/PluginTest.php
@@ -37,20 +40,28 @@ public function testGet() {
     // Without default properties, we get a completely empty plugin definition.

This comment is no longer true! This assertion seems a bit pointless now, maybe replace it with one that checks the exception?

Xano’s picture

Good one! What about this?

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/tests/Drupal/Tests/Component/Annotation/PluginTest.php
@@ -11,6 +11,29 @@
+  /**
+   * @covers ::__construct
+   */
+  public function testConstructWithoutIdShouldError() {
+    $this->expectException(\InvalidArgumentException::class);
+    new Plugin([]);
+  }

We still need to test on PHP 5.6 which for component testing means doing something like

    if (method_exists($this, 'expectException')) {
      $this->expectException(Error::class);
    }
    else {
      $this->setExpectedException(\PHPUnit_Framework_Error::class);
    }

There's no helper here because we're using the PHPUnit TestCase class.

longwave’s picture

Xano’s picture

Status: Needs review » Reviewed & tested by the community

Ah, our components :) Thanks for picking up the fix, @longwave!

Xano’s picture

I added a change record in case anyone runs into this.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Component/Annotation/Plugin.php
@@ -33,6 +33,10 @@ class Plugin implements AnnotationInterface {
+    if (empty($values['id'])) {
+      throw new \InvalidArgumentException(sprintf('Plugin definition %s does not contain a plugin ID.', get_class()));
+    }

Thinking this through a bit more. I think we should trigger a deprecation here and tell people in Drupal 9 we're going to through an exception. That means we don't break any Drupal 8 code and people are warned and then in Drupal 9 we change the @trigger_error() to thrown an exception. It is possible that some contrib/custom tests are doing the same thing as core tests and there's no need to break them in Drupal 8 without warning.

Xano’s picture

That's a fair point. Conceptually, were IDs ever meant to be required? If so, allowing tests to omit IDs continues being a less-than-ideal example, though. What if we move the validation code to a protected method? Child annotations can then override and alter it, allowing IDs to be omitted, for instance, while allowing the base class to behave more like the way we'd like it to. This would also allow annotations to make IDs optional and generate IDs on the fly, for instance. Basically, as long as Plugin outputs an ID, we don't mind where it comes from.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.