API page: https://api.drupal.org/api/drupal/core!lib!Drupal!Component!Plugin!Disco...

> bool $exception_on_invalid: (optional) If TRUE, an invalid plugin ID will throw an exception. Defaults to FALSE.

Looking at the code, this parameter is not optional:

  protected function doGetDefinition(array $definitions, $plugin_id, $exception_on_invalid) {
CommentFileSizeAuthor
#18 2740499-15.patch807 bytesjhodgdon
#16 parameter_to-2740499-11.patch799 bytesnesta_
#16 interdiff-2740499-4-11.txt761 bytesnesta_
#13 2740499-13.patch0 bytesrajeshwari10
#10 parameter_to-2740499-9.patch0 bytesnesta_
#10 interdiff-2740499-4-9.txt761 bytesnesta_
#8 parameter_to-2740499-8.patch761 bytesnesta_
#4 parameter_to-2740499-4.patch753 byteslluvigne
#4 interdiff-2740499-2-4.txt734 byteslluvigne
#2 D8-DiscoveryTrait-2740499-2.patch772 byteser.pushpinderrana
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim created an issue. See original summary.

er.pushpinderrana’s picture

er.pushpinderrana CreditAttribution: er.pushpinderrana as a volunteer and at Publicis Sapient for Publicis Sapient commented
Status: Active » Needs review
FileSize
D8-DiscoveryTrait-2740499-2.patch772 bytes

Removed the optional word but still doubt about "Defaults to False" because there is no default value set for this variable.

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Needs review » Needs work

Thanks for the patch!

'Defaults to False' should be removed too. That only makes sense for an optional variable.

lluvigne’s picture

lluvigne
Primary language Spanish
Location Seville, Spain
CreditAttribution: lluvigne at La Drupalera by Emergya commented
Status: Needs work » Needs review
FileSize
interdiff-2740499-2-4.txt734 bytes
parameter_to-2740499-4.patch753 bytes

Hi!
Acording to @joachim on #3, remove 'Defaults to False' text.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented 
+++ b/core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php
@@ -30,8 +30,7 @@ public function getDefinition($plugin_id, $exception_on_invalid = TRUE) {
+   *   If TRUE, an invalid plugin ID will throw an exception.

This seems slightly incomplete, and leaves me wondering about what happens if it is FALSE.

What if we said something like:

TRUE to throw an exception if the plugin ID is invalid; FALSE to return NULL in this case.

I guess that somewhat overlaps with the @return, but at least it doesn't leave you wondering what happens if it's FALSE. Thoughts?

joachim’s picture

joachim CreditAttribution: joachim commented

How about:

If TRUE, an invalid plugin ID will cause an exception to be thrown, instead of returning NULL.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

The grammar in that suggestion in #6 is a bit questionable -- the two clauses don't have parallel verb tenses: "will cause" vs. "returning". It's also kind of passive voice.

Here are some alternatives, which all at least have parallel grammatical constructions:
- TRUE to throw an exception if the plugin ID is invalid; FALSE to return NULL in this case.
- If TRUE, an invalid plugin ID will cause an exception to be thrown; if FALSE, NULL will be returned.
- Determines what happens if the plugin ID is invalid. TRUE will cause an exception to be thrown; FALSE will cause NULL to be returned.
- Determines what happens if the plugin ID is invalid. If TRUE, an exception will be thrown; if FALSE, NULL will be returned.

Looking at these, I think "cause" is kind of awkward, so let's avoid those alternatives? Other than that, pick one of the above, or come up with another alternative that also has parallel verb grammar. OK?

nesta_’s picture

nesta_ CreditAttribution: nesta_ at La Drupalera by Emergya commented
FileSize
parameter_to-2740499-8.patch761 bytes

fix #4 to #7 :)

Status: Needs review » Needs work

The last submitted patch, 8: parameter_to-2740499-8.patch, failed testing.

nesta_’s picture

nesta_ CreditAttribution: nesta_ at La Drupalera by Emergya commented
Status: Needs work » Needs review
FileSize
interdiff-2740499-4-9.txt761 bytes
parameter_to-2740499-9.patch0 bytes

Sorry. attach valid patch and interdiff

nesta_’s picture

nesta_ CreditAttribution: nesta_ at La Drupalera by Emergya commented

Sorry, bad files Ew!

Status: Needs review » Needs work

The last submitted patch, 10: parameter_to-2740499-9.patch, failed testing.

rajeshwari10’s picture

rajeshwari10 CreditAttribution: rajeshwari10 as a volunteer and at Blisstering Solutions commented
Status: Needs work » Needs review
FileSize
2740499-13.patch0 bytes

Applying Patch.

Thanks!!

Status: Needs review » Needs work

The last submitted patch, 13: 2740499-13.patch, failed testing.

rajeshwari10’s picture

rajeshwari10 CreditAttribution: rajeshwari10 as a volunteer and at Blisstering Solutions commented
Status: Needs work » Needs review
FileSize
2740499-15.patch807 bytes

Sorry for wrong patch applied.

nesta_’s picture

nesta_ CreditAttribution: nesta_ at La Drupalera by Emergya commented
FileSize
interdiff-2740499-4-11.txt761 bytes
parameter_to-2740499-11.patch799 bytes

:) #4 with #7

rajeshwari10’s picture

rajeshwari10 CreditAttribution: rajeshwari10 as a volunteer and at Blisstering Solutions commented

#15 is write patch and # 13 is wrong patch.

Thanks!!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Reviewed & tested by the community
FileSize
2740499-15.patch807 bytes

#16 is the wrong patch, because it has a line that is too long.
#15 looks fine to me.

To avoid the wrong patch being tested/committed, I am uploading the #15 patch again and hiding all the other patches.

Thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed 68cc3af and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed 3288a17 on 8.2.x 
    Issue #2740499 by nesta_, lluvigne, rajeshwari10, jhodgdon, er....

  • alexpott committed 68cc3af on 8.1.x 
    Issue #2740499 by nesta_, lluvigne, rajeshwari10, jhodgdon, er....

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.