Rename Stage to StageBase to clarify its relationship to its subclasses, and add "Stage" suffix to the Updater classes

Per #7.

kunal.sachdev → made their first commit to this issue’s fork.

So, I started to rename things from Stage/stage to StageManager/stage manager but in \Drupal\package_manager\StatusCheckTrait I refactored

* @param \Drupal\package_manager\Stage $stage
*   The stage to run the status check for.

to

* @param \Drupal\package_manager\StageManager $stage_manager
*   The stage manager to run the status check for.

but it's sounding a little vague, there can be more such cases where stage manager wording is not suitable. So, should we think of another name?

Let's discuss with @tedbow & @phenaproxima tomorrow.

Honestly...I like StageManager as a new name for Stage. I think it's not particularly verbose, and it's a lot clearer than stage. It inherently implies certain already-existing design decisions (namely, you have only one stage at any given time).

I'm going to open an MR to rename it.

WFM! But there's still some unaddressed problems then:

1. Many comments no longer make sense — see MR for details.
2. We should rename \Drupal\package_manager\StageManager::getStagingRoot() to ::getRoot()
3. We should rename \Drupal\package_manager\PathLocator::getStagingRoot() to ::getStageRoot()

Many comments no longer make sense

Yeah, this is gonna be messy.

Agreed on the method renames, and implemented.

Yeah, this is gonna be messy.

Messy today, virtually impossible once it's in core! 😅 So it's the right kind of mess/pain 😄

I think I've fixed most instances of "stage" across Package Manager and Automatic Updates.

No doubt there are still some stragglers here and there which evaded my heuristics, but those should be easy to clean up later. I'm guessing they are all just in comments. This MR renames Stage to StageManager, and TestStage to TestStageManager.

Checked it out locally and searched in PHPStorm for the stage\W regex to find remaining "stage" occurrences.

99% of them made sense 👏

Stragglers:

1. StageManager $stage should become StageManager $stage_manager everywhere.

2.  * \Drupal\package_manager\Event\PreOperationStageEvent. All events have a
    * $stage property which allows access to the stage object itself.
   
   

   → this is where point 1 actually affects the public API in a perhaps unexpected way!

3. $stage = $this->updater;
   

   $stage instanceof
   

   i.e. whenever $stage is the variable/property name, but it's not "the stage", but instead "the stage manager".

5. // We only want to do this check if the stage belongs to Automatic Updates.
   

   → similarly: stage manager, not stage. But this is the only such remaining comment reference I could find! 👏 (It occurs in three files though.)

🏓 Reviewed using git diff 4ea1985d0beeb0d401bc1adefa7af1b459c9adfc HEAD.

It's thanks to the challenges I spotted during this review that I thought of an alternative approach:

1. An abstract basic stage with no specializations: Stage(Manager) → abstract BasicStage or SimpleStage
2. Non-abstract specialized stages: Updater → UpdateStage and CronUpdater → CronUpdateStage
3. Only \Drupal\package_manager\PackageManagerUninstallValidator and \Drupal\package_manager_test_api\ApiController::create() instantiate a Stage(Manager), we could easily provide an anonymous class in both places to overcome that challenge.

I think that'd actually be far closer to the reality we want? We would then:

1. only ever use specialized "stages", except in one test and a very abstract/generic uninstallation validator
2. still be able to keep all the existing docs about "The stage for which to fire an event" etc — it'll just then be very clear that the PHP type declaration refers to the basic stage, but only a concrete/specalized stage can be passed in

👆 This is exactly what we have for (Content)EntityBase!

Because \Drupal\project_browser\ComposerInstaller\Installer can then become InstallerStage.

1. re #23 I am not opposed to making \Drupal\package_manager\Stage an abstract call and changing Updater → UpdateStage and CronUpdater → CronUpdateStage it seems like a good thing to do.
2. Regarding the current use of Stage directly in \Drupal\package_manager\PackageManagerUninstallValidator. I think it would be better here to make a non-abstract AvailabilityCheckerStage or something along those lines which would only allow calling isAvailable() and isApply() so that it would very clear how you can check these things if you wanted to stop other operations from happening while Package Manager was applying. I think us making a non-test anonymous class would set a bad example because someone could see that example and think "oh if I just want require a package real quick I could also just make anonymous class". So we would have to document why our use of anonymous class was ok but for other operations it is bad idea(assuming it is). I think AvailabilityCheckerStage would be clearer
3. Just having the abstract Stage class though doesn't really solve the problems stated in the summary
   • "stage" is both a noun and a verb, but inconsistent with the above
   • it's too closely aligned with "stage directory", "staging area", etc

   I think maybe it just proves that StageManager is not that much better

I would suggest we create another issue to

1. Stage abstract
2. create AvailabilityCheckerStage which would get rid of 1 non-test anonymous stage use.
3. rename Updater → UpdateStage and CronUpdater → CronUpdateStage

Then we just leave this issue open because I still think we have solved the naming problem of "stage" as described in the summary. Maybe as we get more fresh eyes in the this review process someone will come up with a more obvious name

Then why not just change class Stage to abstract class StageBase?

That’s what I actually already proposed in my previous comment: abstract class BasicStage or abstract class SimpleStage.

All three of these choices make it clear it’s a noun and not a verb.

Then why not just change class Stage to abstract class StageBase?

That sounds good to me plus a new AvailabilityCheckerStage for PackageManagerUninstallValidator

In #3326486-24: Rename \Drupal\package_manager\Stage to StageManager for clarity and consistency → .2 how we would allow it to only call isAvailable() and isApply().

Discussed with @tedbow and @phenaproxima about #24 📌 Rename Stage to StageBase to clarify its relationship to its subclasses, and add "Stage" suffix to the Updater classes Fixed .2 and got to a conclusion that for now we can have anonymous class as suggested by @Wim Leers in #23 📌 Rename Stage to StageBase to clarify its relationship to its subclasses, and add "Stage" suffix to the Updater classes Fixed .3

🥳 Glad to see this happening! 🚀

• \Drupal\Tests\automatic_updates_extensions\Kernel\ExtensionUpdaterTest
• \Drupal\Tests\package_manager\Unit\StageTest
• \Drupal\Tests\package_manager\Kernel\StageTest
• \Drupal\Tests\automatic_updates\Kernel\TestCronUpdater and \Drupal\Tests\automatic_updates\Kernel\CronUpdaterTest

should also be renamed. Other than that I can confirm all occurrences of
ExtensionUpdater are replaced with ExtensionUpdateStage,
Stage are replaced with StageBase,
Updater are replaced with UpdateStage and
CronUpdater are replaced with CronUpdateStage.

Now this looks more consistent to me.

Ooof, 74 files changed...! Self-assigning to review.

As I looked at this, I felt that the class renaming is so effective that, indeed, things like ExtensionUpdateStage $extensionUpdateStage are redundant! I think it would be clear to say ExtensionUpdateStage $stage, and it'd be quite clear that this is a type of stage.

But, just to sanity check myself, I proposed this to @tedbow as well in a Zoom conversation, and he agreed that the type hints are so much clearer that we can remove verbose, redundant variable and method names. So that's what I'm going to do in my review -- point out the places where we could rename these.

That said, this is great work and I think it's a good solution to this problem. Well done all!

Looks great!

One or two small things, then this is good to go from a code perspective. Apart from that, this only needs the follow-up @tedbow requested in #24 to be filed, at which point this is RTBC.

Love it.

RTBC once the follow-up is open.

In discussion with @kunal.sachdev and @tedbow in Slack, we came to the conclusion that we don't need the follow-up to create AvailabilityCheckerStage, because it's probably overkill at the moment. The only place we need to generically check stage availability is in Package Manager's uninstall validator, and the anonymous class is fine for that right now.

phenaproxima → closed merge request !705

🚀

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