Update documentation for project versions in \Drupal\Core\Extension\InfoParserInterface::parse

+++ b/core/lib/Drupal/Core/Extension/InfoParserInterface.php
@@ -34,7 +34,7 @@
-   *   - (versions): Version information, consisting of one or more
+   *   - (versions): Optional version information, consisting of one or more

Normally to indicate that something is optional in a list, the syntax is:

* - key: (optional) Information goes here

In this case we have

* - (key): (optional) Information goes here

Which could be confusing since it implies (optional) is part of the string to appear in the file. For the case when the key is enclosed in parenthesis the following probably makes the break between the file syntax and description of the syntax clearer

* - (key): Optional key, information goes here

This is followed elsewhere so it is not new. So I think the current patch is fine.

This (optional) is the standard way to do this. See:
https://www.drupal.org/node/1354#lists

Please review attached one.

Thanks! Closer...

+++ b/core/lib/Drupal/Core/Extension/InfoParserInterface.php
@@ -34,7 +34,7 @@
+   *   - versions: (Optional) Version information, consisting of one or more

(optional) should be lower-case by convention.

Also in this case "(versions)" is what is actually in the string here, and you'd need to omit that whole piece for this to work, so I think we should leave the parens on (versions) here?

You mean to say only

+++ b/core/lib/Drupal/Core/Extension/InfoParserInterface.php
@@ -34,7 +34,7 @@
+   *   - (versions): 

will work ?

Should say:

* - (versions): (optional) Blah blah blah etc.

Done. Thanks !!

Thanks!

As a docs-only fix, this change is rc eligible. Reference: https://www.drupal.org/core/d8-allowed-changes#rc

1. --- a/core/lib/Drupal/Core/Extension/InfoParserInterface.php
   +++ b/core/lib/Drupal/Core/Extension/InfoParserInterface.php
   
   @@ -34,7 +34,7 @@
   -   *   - (versions): Version information, consisting of one or more
   +   *   - (versions): (optional) Version information, consisting of one or more
   

   The array key (versions) is really wrapped in parentheses? Huh. :)

2. +++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
   @@ -637,8 +637,10 @@ protected function verifyImplementations(&$implementations, $hook) {
   -   *   An associative array with three keys:
   +   *   An associative array with either three or four keys:
       *   - 'name' includes the name of the thing to depend on (e.g. 'foo').
   +   *   - 'project' (if present) contains the name of the project that contains
   +   *     the thing to depend on.
       *   - 'original_version' contains the original version string (which can be
       *     used in the UI for reporting incompatibilities).
       *   - 'versions' is a list of associative arrays, each containing the keys
   

   Shouldn't this also use our standard format for associative arrays? E.g.:

   - name: The name of the thing to depend on (e.g. 'foo').
   - project: (optional) The name of the project.... (etc.)
   

NW for #2; it seems to be in the scope stated in the issue title. Thanks!

Good idea! Might as well make the docs even better. :)

Done Changes

Doing some testbot maintenance and need to kill the test run in #14. Since it is only affecting comments, I assume that's fine.

Thanks! The changes in ModuleHandler look good. But:

+++ b/core/lib/Drupal/Core/Extension/InfoParserInterface.php
@@ -34,7 +34,7 @@
+   *   - (versions): optional Version information, consisting of one or more

The patch in #10 was correct in this line. This patch isn't. Please go back to the patch in #10 version.

Done.

+++ b/core/lib/Drupal/Core/Extension/InfoParserInterface.php
@@ -34,7 +34,7 @@
+   *   - versions: (optional) Version information, consisting of one or more

This line is still wrong. Again, please go back to the patch in #10 for this line.

Thanks! This looks fine. We could change
@return
to
@return array
in the second hunk, but at least this documentation is better.

+++ b/core/lib/Drupal/Core/Extension/ModuleHandler.php
@@ -637,11 +637,13 @@ protected function verifyImplementations(&$implementations, $hook) {
+   *   - project: (optional) The name of the project that contains
+   *     the thing to depend on.

I'm not sure (optional) makes much sense in an @return - I think we need to document why it is present when it is.

We don't really need that (optional) .

Hi All,
In the API (https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Extension!ModuleH...), It state that 3 values are returned,so Have removed optional key.

I'm sorry, but I don't think either of the two patches here is correct.

If you look at the code linked in #29, you'll see in the first few lines:

// Split out the optional project name.
  if (strpos($dependency, ':') !== FALSE) {
    list($project_name, $dependency) = explode(':', $dependency);
    $value['project'] = $project_name;
  }

So definitely 'project' can be returned, but it is not always returned. So the patch in #29 is incorrect.

The patch in #27 is also unclear. It says it can return 3 or 4 keys, which is correct, but it doesn't say which one is sometimes not there or why. In #26, @alexpott requested that we not use (optional) in there to describe this, which I agree with. But we still need to put something in there to tell people that project might be missing, and why.

Also, whoever is working on the next patch, please start by assigning the issue to yourself first, so that we don't have two people submitting patches on the same issue. That is a waste of time, both for people making the patches and for reviewers. Thanks!

The documentation that was in the ModuleHandler is now in \Drupal\Core\Extension\Dependency::createFromString and has since been updated. I don't think that requires a change anymore. What is left is simply to add (optional). I have updated the issue summary

Hi all,
I have created a new MR !11528 with the necessary documentation update.

This is now ready for review. Let me know if any further adjustments are needed.

Thanks!

The change is simple and LGTM. Seems the error is unrelated through im not able to rerun the job?

This needs another adjustment to meet coding standards.

Updated to reflect comment #55.

Seems pretty straight forward.

Nice to get this finally fixed!