Improve documentation for Graph component

This might be something a novice can progress

There's some uses of this in core for sorting module dependencies and also in default content module for sorting imports

Please hold this issue for first time contributors at DrupalCon Prague 2022.

Please check if this is correct.

please ignore patch on comment #7.

Please see this one.

Hi chaubeyji, whilst everyone appreciates contributions and moving these issues forwards, in comment #6 ultimike had requested that this issue be held for Prague 2022 DrupalCon first time contributors.

I would encourage the reading of the issue comment thread prior to updating to make sure this is avoided as a lot of time is put into doing issue triage to identify these for the events.

The review of the patch can still be worked on by the first time contributors.

Thank you @kimberleycgm, I am a first time contributor, thank you for comment, I will keep it in mind.

Thank you!

I think the docblock still needs more of a rewrite.

> Return? This is a constructor. It doesn't return anything. What does this mean?

My guess is that at some point this object was refactored, and the documentation didn't get a matching refactor. The constructor says it instantiates the "depth first search" object. It probably used to contain the code that's now in ::searchAndSort(), and when that got moved to its own method, the docblock didn't move along with it. So the constructor docblock should instead start out with "Instantiates the directed acyclic graph object."

For the description under @param $graph, I'd say something more like

A three-dimensional associative array, keyed by the names of the graph's vertices, which can be strings or numbers. Each vertex may have an 'edges' key, whose value is an array keyed by the names of the vertices connected to it; the values in this array can be simply TRUE or may contain other data.

The example in the code block is good—vital even—but I think it should be preceded by a little diagram of the graph, like the one in the comments at \Drupal\Tests\Component\Graph\GraphTest::testDepthFirstSearch(). For the example given, the diagram would be

1 --> 2 --> 3
      |     |
      |     v
      + --> 4

or if we can use ASCII Extended in docblocks,

1────►2────►3
      │     │
      │     ▼
      └───► 4

The second @code block in the constructor—the one with 'paths' and 'reverse_paths' keys—should be moved to the ::searchAndSort() method, because it's describing the return value of ::searchandSort() if you pass it the graph described above.

Updating the remaining tasks section.

Hi, I've made MR 8124 which is ready for review. I've added both the patch from #9 and the changes in #16.

Thanks

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Thanks for working on this! It looks like the diagram needs an update—the code and the diagram don't currently match. I think lines 28-30 need to be indented, so that 2 and 3 lead to 4, not 1 and 2 leading to 4.

Nice spot @ksenzee!

Made that adjustment to the MR and should be ready for review!

Failure is unrelated.

Appears feedback has been addressed.

@NexusNovaz, thank you for working on this issue! And for you prompt replies to feedback.

I have reviewed the MR and don't think that the example should be split into two parts, one in the constructor and one in the method searchAndSort. Instead, let's use the example of migrate process plugins where the examples are in the Class doc bloc. See MigrationLookup for one example.

Hi @quietone

The previous MR (!8124) was quite outdated and significantly behind the 11.x branch. I initially attempted a rebase, but due to a large number of conflicts, it became difficult to manage cleanly.

To ensure a clean and conflict-free update, I’ve created a new branch and opened this fresh MR (!11606) with the necessary changes, fully rebased with the latest from 11.x.

All tests are now passing. Let me know if anything else is needed!

Thanks!

Feedback to move example to class block seems to been addressed.

@brandonlira, yes, thanks for the explanation. Have you seen rebase a merge request?

Thanks everyone for improving the documentation.!

Restoring attribution.