Allow DiscoveryInterface::getDefinition() to throw an exception for an invalid plugin

Meaning we'd need a hasDefinition() method for simple existence checks ?

Not having an argument at all would require that method. But having that method might be a good idea anyway, as if ($manager->hasDefininition()) makes more sense than getDefinition().

I'd vote for having the argument and default to TRUE (throwing exception). That's the same behavior as $container->get().

Consider making it default to TRUE, or just making that the actual expected behavior, and never return NULL

A non existing plugin is certainly not the code path you expect to have, so an exception is a good tool here.

+++ b/core/modules/edit/lib/Drupal/edit/EditController.php
@@ -108,7 +108,7 @@ public function metadata(Request $request) {
-      if (!$entity_type || !$this->entityManager()->getDefinition($entity_type)) {
+      if (!$entity_type || !$this->entityManager()->getDefinition($entity_type, FALSE)) {

+++ b/core/modules/entity_reference/entity_reference.module
@@ -198,7 +198,7 @@ function entity_reference_query_entity_reference_alter(AlterableInterface $query
-  if (!\Drupal::service('plugin.manager.field.field_type')->getDefinition('entity_reference')) {
+  if (!\Drupal::service('plugin.manager.field.field_type')->getDefinition('entity_reference', FALSE)) {

+++ b/core/modules/field/field.views.inc
@@ -112,7 +112,7 @@ function field_views_field_default_views_data(FieldInterface $field) {
-  if (!\Drupal::service('plugin.manager.field.field_type')->getDefinition($field->getType())) {
+  if (!\Drupal::service('plugin.manager.field.field_type')->getDefinition($field->getType(), FALSE)) {

+++ b/core/modules/field/lib/Drupal/field/FieldInfo.php
@@ -438,7 +438,7 @@ public function getBundleInstances($entity_type, $bundle) {
-    if (\Drupal::entityManager()->getDefinition($entity_type) && !empty($field_map[$entity_type])) {
+    if (\Drupal::entityManager()->getDefinition($entity_type, FALSE) && !empty($field_map[$entity_type])) {

+++ b/core/modules/edit/lib/Drupal/edit/Access/EditEntityAccessCheck.php
@@ -64,7 +64,7 @@ protected function validateAndUpcastRequestAttributes(Request $request) {
-      if (!$entity_type || !$this->entityManager->getDefinition($entity_type)) {
+      if (!$entity_type || !$this->entityManager->getDefinition($entity_type, FALSE)) {

+++ b/core/modules/edit/lib/Drupal/edit/Access/EditEntityFieldAccessCheck.php
@@ -64,7 +64,7 @@ protected function validateAndUpcastRequestAttributes(Request $request) {
-      if (!$entity_type || !$this->entityManager->getDefinition($entity_type)) {
+      if (!$entity_type || !$this->entityManager->getDefinition($entity_type, FALSE)) {

Still +1 on a hasDefinition($plugin_id) ?

+++ b/core/lib/Drupal/Core/Field/FieldTypePluginManager.php
@@ -104,4 +105,25 @@ public function getConfigurableDefinitions() {
+  public function getDefinition($plugin_id, $exception_on_invalid = TRUE) {
...
+    throw new FieldException(format_string('Attempt to create a field of unknown type %type.', array('%type' => $plugin_id)));

+++ b/core/modules/field/lib/Drupal/field/Entity/Field.php
@@ -327,9 +327,6 @@ protected function preSaveNew(EntityStorageControllerInterface $storage_controll
-    if (!$field_type) {
-      throw new FieldException(format_string('Attempt to create a field of unknown type %type.', array('%type' => $this->type)));
-    }

Hm - not sure why we'd want to do that ?
Plus, there might be places that call FieldTypePM::getDefinition($unknow_plugin_id) but are not "attempting to create a field" ?

I'd tend to leave FieldTypePM::getDefinition() untouched / non-overriden, and have Field::preSaveNew() catch the generic plugin exception and throw its more specific one ?

Seems like I've seen this code before. I was leaning toward just making this the default functionality but that would be bad for derivatives.

So, seems like this is a useful addition. +1

I will call out some things I don't like here, namely the addition of yet more base class things to inherit from. Traits would really clean this up, and while I know we all know that, I'm going to call it out so that we have yet another place where traits could make things less WTF. That being said the basic functionality here is very sensible to me.

A few nitpicks, otherwise I'm generally ++ to this.

1. +++ b/core/modules/datetime/lib/Drupal/datetime/Plugin/Field/FieldType/DateTimeItem.php
   @@ -54,7 +54,7 @@ public function getPropertyDefinitions() {
   -      static::$propertyDefinitions['date'] = DataDefinition::create('datetime_computed')
   +      static::$propertyDefinitions['date'] = DataDefinition::create('any')
   

   Why'd this change?

2. +++ b/core/modules/field/tests/modules/field_test/field_test.info.yml
   @@ -5,3 +5,5 @@ core: 8.x
   +dependencies:
   +  - entity_test
   

   What about this patch caused new dependencies to be needed for this test?

3. +++ b/core/modules/system/tests/modules/entity_test/entity_test.install
   @@ -32,7 +32,7 @@ function entity_test_install() {
   -      ->setComponent('field_test_text', array('type' => 'text_text'))
   +      ->setComponent('field_test_text', array('type' => 'text_textfield'))
   

   Why'd this change?

Not going to change the status cause these could be completely justified. Please just gimme a couple explanations here and I'd be happy to rtbc this.

Eclipse

That is perfectly sensible.

Eclipse

+1 to RTBC

Still have the thoughts/concerns from #29 but as Tim said they make things worse right now so this is the correct approach.

discovery-2178795-40.patch no longer applies.

error: patch failed: core/modules/field/field.views.inc:67
error: core/modules/field/field.views.inc: patch does not apply

Re-roll, and I made the following minor change while re-rolling this particular line:
From:

  if ($entity_manager->getDefinition($field->entity_type) && $entity_manager->getStorageController($field->entity_type) instanceof FieldableDatabaseStorageController) {
    return TRUE;
  }

to:

  return $entity_manager->hasDefinition($field->entity_type) && $entity_manager->getStorageController($field->entity_type) instanceof FieldableDatabaseStorageController;

Agreed, that is appropriate. The docs only document the true case and null is falsy but this is more correct for returning a bool.

drupal_2178795_46.patch no longer applies.

error: patch failed: core/modules/datetime/lib/Drupal/datetime/Plugin/Field/FieldType/DateTimeItem.php:54
error: core/modules/datetime/lib/Drupal/datetime/Plugin/Field/FieldType/DateTimeItem.php: patch does not apply

Re-roll, conflicted in DateTimeItem.

applies fine.

1. +++ b/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryInterface.php
   @@ -18,11 +18,18 @@
   +   * @param bool $exception_on_invalid
   +   *   (optional) If TRUE, an invalid plugin ID will throw an exception.
   +   *   Defaults to FALSE.
   ...
   +  public function getDefinition($plugin_id, $exception_on_invalid = TRUE);
   

   defaults to true. Also we dont have to give the defaults anymore (we used to, but dont now) according to 1354. https://drupal.org/node/1354#param

2. +++ b/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryInterface.php
   @@ -18,11 +18,18 @@
       * @return mixed
   -   *   A plugin definition, or NULL if no definition was found for $plugin_id.
   +   *   A plugin definition, or NULL if the plugin ID is invalid and
   +   *   $exception_on_invalid is TRUE.
   
   +++ b/core/lib/Drupal/Component/Plugin/Discovery/StaticDiscoveryDecorator.php
   @@ -41,14 +41,14 @@ public function __construct(DiscoveryInterface $decorated, $registerDefinitions
   -  public function getDefinition($base_plugin_id) {
   
   +++ b/core/lib/Drupal/Component/Plugin/PluginManagerBase.php
   @@ -8,11 +8,12 @@
     */
   

   if an exception is thrown, then uh... it wont be able to return, right? so I'm thinking this is NULL if it's invalid and exception_on_invalid is FALSE.

3. +++ b/core/lib/Drupal/Core/Entity/EntityManagerInterface.php
   @@ -195,28 +195,16 @@ public function getBundleInfo($entity_type);
   +   * {@inheritdoc}
       *
       * @return \Drupal\Core\Entity\EntityTypeInterface|null
   ...
      /**
   -   * Returns an array of entity type info, keyed by entity type name.
   +   * {@inheritdoc}
       *
       * @return \Drupal\Core\Entity\EntityTypeInterface[]
   -   *   An array of entity type objects.
       */
      public function getDefinitions();
   

   why are these leaving the @return? should just be fine with {@inheritdoc}, right?

here is a patch that just takes out that (incorrect) defaults to, and fixes the return details.

Probably need to declare and abstract getDefinitions() method on the DiscoveryTrait.

Eclipse

+++ b/core/modules/entity_reference/entity_reference.module
@@ -204,7 +204,7 @@ function entity_reference_query_entity_reference_alter(AlterableInterface $query
  */
 function entity_reference_create_instance($entity_type, $bundle, $field_name, $field_label, $target_entity_type, $selection_handler = 'default', $selection_handler_settings = array(), $cardinality = 1) {
   // If a field type we know should exist isn't found, clear the field cache.
-  if (!\Drupal::service('plugin.manager.field.field_type')->getDefinition('entity_reference')) {
+  if (!\Drupal::service('plugin.manager.field.field_type')->hasDefinition('entity_reference')) {
     field_cache_clear();
   }

Pretty sure that this is dead code now with the standardized plugin cache clearer...

I removed the optional exceptions like we did back in #1846070: Improve exception messages when plugins cannot be found, because if you don't want an exception, you should have used hasDefinition() first. This way we also don't have to change the interface, and we keep our API simple and consistent.

if you don't want an exception, you should have used hasDefinition()

+1, most sane and most sensible approach.

That was the direction of the OP. See #1 and #2.

Not everybody agrees on that approach, so I wanted to propose an alternative.

Also, your patch makes zero changes to the actual calling code, if you want to hijack an issue at least make the necessary changes correctly.

I didn't want to spend time on something that did not seem important for demonstrating my proposal. If the approach I suggested will be accepted, then I will gladly finish the conversion. Please be careful when you tell people they hijacked issues. It's not motivating.

I'll work on an improved version of #58.

This patch follows #58.

Ah, ok, so #62 was not pursued and back to getting #58 to pass. ok.

1. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageFieldsTest.php
   @@ -112,7 +112,7 @@ function manageFieldsPage($type = '') {
   -    $this->assertIdentical("$url/delete", (string) $result[2]['href']);
   +    $this->assertIdentical("$url/delete", (string) $result[3]['href']);
   

   why? what is result[2] now?

2. +++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/d6/MigrateCckFieldRevisionTest.php
   @@ -19,7 +19,7 @@ class MigrateCckFieldRevisionTest extends MigrateNodeTestBase {
   -  public static $modules = array('node', 'text');
   +  public static $modules = array('field', 'filter', 'node', 'text');
   
   +++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/DisplayFeedTest.php
   @@ -26,7 +26,7 @@ class DisplayFeedTest extends PluginTestBase {
   -  public static $modules = array('node', 'views_ui');
   +  public static $modules = array('block', 'node', 'views_ui');
   

   why is *this* issue revealing that we need to declare that migrate and display tests need to enable field, filter and block?

   field kinda makes sense, but filter and block? because they provide fields....? that were expected? but when they were not there, null was returned before and now we are throwing exception? so we had tests that were not really testing what we thought they were?

why? what is result[2] now?

Because entity_test.module is enabled now, it adds an additional entity operation between the other ones. I'm not sure the test should test for the order of operations. Should we just assert whether the correct URL exists on the page instead, or is order really important here?

why is *this* issue revealing that we need to declare that migrate and display tests need to enable field, filter and block?

Many of the other fails were because field_test uses the 'entity_test' entity type, but doesn't always enable entity_test.module. How that works is beyond me.

-@tim.plunkett in #9.
This may be related. I found out the test needed Text's field, and as Text depends on Field and Filter, and Simpletest does not enable dependencies automatically, I had to add all of them here.

+++ b/core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/d6/MigrateCckFieldRevisionTest.php
@@ -19,7 +19,7 @@ class MigrateCckFieldRevisionTest extends MigrateNodeTestBase {
diff --git a/core/modules/simpletest/files/.htaccess b/core/modules/simpletest/files/.htaccess

oops I this should not have been included.

-----
Novice task to make a new patch to take out that file. (the issue is not novice but making a new patch with just the needed files is)
https://drupal.org/contributor-tasks/create-patch

I'll do #72

Just Re-rolled #69 and deleted the .htaccess file from core/modules/simpletest/files/ in #69 as @YesCT said in #72

Thanks!

removing novice tag since that one novice task was done. thanks @jlbellido

Seems like the remaining task here is a review.

looked at the diff of the last couple patches and it seems @jlbellido added a blank line accidentally. removing that.

looked at this whole thing again, rtbc from me.

just some comments as i read through, if bad ones please disregard

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -312,7 +312,7 @@ public function getController($entity_type, $controller_type, $controller_class_
   +    if (($entity_type = $this->getDefinition($entity_type_id, FALSE)) && $admin_form = $entity_type->getLinkTemplate('admin-form')) {
   

   why the () around $entity_type = .. ? i don't think you need them?

2. +++ b/core/lib/Drupal/Core/Entity/EntityManagerInterface.php
   @@ -274,28 +274,16 @@ public function getExtraFields($entity_type_id, $bundle);
   +  public function getDefinition($entity_type_id, $exception_on_invalid = TRUE);
   

   you changed the false to true here, is that fine?

I had a pretty hilarious exchange in IRC with timplunkett while reading this patch where I basically repeated everything I said about not liking the optional parameter in #2168333-54: Ensure custom EntityType controllers can be provided by modules. Oh, well, at least I'm consistent, even if I have the memory of a goldfish. :P~

It doesn't really make sense though to have one type of plugin use this method of error trapping and the rest not, given this has buy-in from all of the plugin subsystem maintainers from what I can see. The DIscoveryTrait is nice. I also like the fact that it defaults to TRUE (meaning, show the exception), which IMO is what you'd expect. I'm still a bit worried about what this will do to existing contrib modules out there though, since that FALSE parameter had to be stuck in some pretty innocuous places in the patch.

Nevertheless, I think this is good to go in, since it makes an existing situation in HEAD a tad bit nicer/more consistent.

Committed and pushed to 8.x. Thanks!

Thanks!

Committed and pushed hotfix to 8.x.

Already committed.