Define the Package Manager API (package_manager.api.php is outdated)

create 📌 Update Package Manager event documentation in package_manager.api.php Fixed as a child issue so we can tackle a small part of this

👍 Let's avoid conflicting with that issue! 😄

📌 Update Package Manager event documentation in package_manager.api.php Fixed landed, this is now unblocked 👍

Given #3341469-2: Improve StatusCheckEvent subscriber DX for determining if there is a staged update → , I'll take this on.

Waiting for 📌 Mark FailureMarker @internal and document ComposerUtility, PathLocator and ValidationResult Fixed to land first…

Blocker is in!

Trying to adjust credit given that the patch in #4 makes no actual changes to the documentation.

Mirrored the structure of core/modules/ckeditor5/ckeditor5.api.php, the corresponding output of which can be seen at https://api.drupal.org/api/drupal/core%21modules%21ckeditor5%21ckeditor5....

Self-review for context

+++ b/package_manager/package_manager.api.php
@@ -152,13 +167,16 @@
+ * @section sec_validators_excluders API: excluders vs validators & status checks

@@ -185,23 +203,32 @@
- * \Drupal\package_manager\Event\StatusCheckEvent
- * Dispatched to check the status of the Drupal site and whether Package Manager
- * can function properly. These checks can be performed anytime, so this event
- * may be dispatched multiple times.
...
+ *
+ * Virtually every validator should react not only to
+ * \Drupal\package_manager\Event\PreCreateEvent and/or
+ * \Drupal\package_manager\Event\PreApplyEvent but also
+ * \Drupal\package_manager\Event\StatusCheckEvent. This event is dispatched to
+ * check the status of the Drupal site and whether Package Manager can function
+ * properly.
+ *
+ * The Package Manager module does not trigger this event because it does not
+ * have a UI. Modules building on top of Package Manager are likely to dispatch
+ * this event to prevent the user from starting an operation that would fail.
+ *

These are arguably the most contentious bits.

See #3341469-7: Improve StatusCheckEvent subscriber DX for determining if there is a staged update → and preceding discussion.

I tried to clarify here what I thought in #3341469-2: Improve StatusCheckEvent subscriber DX for determining if there is a staged update → to be wrong, which @phenaproxima said at #3341469-4: Improve StatusCheckEvent subscriber DX for determining if there is a staged update → to be correct, but then my detailed investigation at #3341469-7: Improve StatusCheckEvent subscriber DX for determining if there is a staged update → showed that it was actually wrong, just in a pretty subtle way.

I tried to make this clear once and for all by documenting it like this — hope y'all like it 😊

This looks pretty good to me, but I'm not sure I should give the final sign-off. For the simple reason that I'm too close to this code. I've been living and breathing Package Manager for over a year. Almost any explanation of its internals will make sense to me, because my mind will automatically fill in the gaps.

I think I want this to be reviewed by someone with much less familiarity with the internals before I commit it.

Almost any explanation of its internals will make sense to me, because my mind will automatically fill in the gaps.

Looks like I missed my opportunity to get lorem ipsum committed as official docs 🤣🤣🤣

I'm glad that at least you found nothing wrong! 🥳

But if you can't commit it, then only @tedbow can IMHO. Given his role on this project, I think that probably makes sense anyway 👍

Don't get me wrong: I'm happy to commit it. I did read the diff and the changes made sense.

I merely want a second pair of eyes to review it first, a set of eyes that hasn't had my level of exposure to Package Manager.

Hi all, just chiming in here. I downloaded 3.0.x, applied the patch, and just read package_manager.api.php from top to bottom.

Overall, this absolutely gives me an understanding of how Package Manager works. I had only three things come to mind as I read it:

1. Near line 35: The piece about "only using the same class..." seems like an oddity to me, so might need some explanation why (although that documentation may not need to be in .api.php, but rather in a handbook page or something).
2. Near line 120: I understand event propagation in the JS world, so stopping propagation in that sense makes sense to me. I don't know how to stop propagation, though (is there a Drupal API call EventSubscriber::stopPropagation() or something?), so pointing more specifically to a code example would be good. Also, if you are expecting a consumer/user of Package Manager to have to do the stopping, this is even more important.
3. ...and for the stupid part... lines 160, 170, and 231 go past 80 characters. :)

This is great feedback.

Near line 35: The piece about "only using the same class..." seems like an oddity to me, so might need some explanation why (although that documentation may not need to be in .api.php, but rather in a handbook page or something).

I believe you're referring to:

Only the owner can perform
 * operations on the stage directory, and only using the same class (i.e.,
 * \Drupal\package_manager\Stage or a subclass) they used to create it.

I can see why this would seem odd, and maybe we can clarify it. What it comes down to is that, for Package Manager, "ownership" consists of two things -- a random token, and the fully qualified name of the Stage that you called create() on. You need both of these things in order to reclaim the stage in subsequent requests and do things to it.

Near line 120: I understand event propagation in the JS world, so stopping propagation in that sense makes sense to me. I don't know how to stop propagation, though (is there a Drupal API call EventSubscriber::stopPropagation() or something?), so pointing more specifically to a code example would be good. Also, if you are expecting a consumer/user of Package Manager to have to do the stopping, this is even more important.

subclass

Yeah, we would expect code that integrates with Package Manager (a custom validator or event subscriber) to stop propagation if they encounter some catastrophic condition that's going to break any further subscribers. Symfony Event objects have a stopPropagation() method we would expect them to call, so let's add a reference to that.

And while we're at it, let's fix the longer-than-80-characters lines. :)

Addressing the feedback, and adding the docs that 📌 Add new InstalledPackagesList which does not rely on Composer API to get package info Fixed should've added.

Also removing the docs that #3344039 should have removed (see #3344039-20: Add a validate() method to ComposerInspector to ensure that Composer is usable → ).

Now waiting for 📌 Create an API for base requirement validators which run before other validators and stop event propagation if they fail Fixed to land, because that is changing this file too.

Adam said he'd take this on! 🥳

Went over this from top to bottom and I reorganized and pushed and pulled until I think it's both accurate and comprehensive.

I'd like review from Wim, since he's great at clarity and accuracy...and I'd also like a review from a consumer of Package Manager (that means @bnjmnm or @chrisfromredfin) before we commit.

Review posted.

I don't think we need +1s from Package Manager consumers because this clearly is a massive leap forward. Any further improvements can happen at a later time — this is actively blocking the core alpha-experimental MR!

🏓

A huge leap forward, go for launch! 🚀

Whew!

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