Decide which classes should be internal and/or final — delete ExcludedPathsTrait, make CollectPathsToExcludeEvent richer

📌 Mark FailureMarker @internal and document ComposerUtility, PathLocator and ValidationResult Fixed just marked FailureMarker internal. Why shouldn't it be @internal?

Generally speaking, it requires @phenaproxima and @tedbow to do this I think, I don't think we can ask Yash, Omkar or Kunal to take this on. I could do it perhaps, but not knowing the full history/intent, I think it'd be better for me to be the reviewer?

I think this can be core-post-mvp based our recent discussion what is truly alpha blockers for core. I have stressed when talking to committers that if we can't break BC in Alpha in core, as is the policy, we shouldn't be trying to get into core so quickly

I think all of the API refactoring stuff is out of the way for 3.0.x at this point, with the sole exception of 📌 Rename Stage to StageBase to clarify its relationship to its subclasses, and add "Stage" suffix to the Updater classes Fixed . But that's just a rename. We can and should start doing this now!

IMHO, the stuff that should not be internal and/or final:

• PathLocator
• ComposerInspector
• Stage(Manager)
• PackageManagerKernelTestBase
• Everything in Drupal\package_manager\Event
• PathExclusionsTrait
• InstalledPackagesList and InstalledPackage (final, but not internal)
• InstalledPackage

Everything else should be explicitly internal and, unless it needs to be subclassed for testing purposes, final.

#10 sounds like a great starting point. I agree mostly, but disagree about a few:

1. I think we should keep PackageManagerKernelTestBase internal. You're free to use it, but you'll have to chase test changes. That's the only way we can keep evolving it.
2. I disagree everything in Drupal\package_manager\Event should not be internal. For example, RequireEventTrait makes no sense to not be internal. Similarly, I believe we should remove Event/ExcludedPathsTrait and just move those few lines of code into the concrete event classes. 👉 Did that in e8632194f03411dc1a34989803079cdd25728760.
3. I disagree about PathExclusionsTrait — I think instead that logic should be moved into CollectIgnoredPathsEvent itself — because these convenience methods make sense for every subscriber of the CollectIgnoredPathsEvent event, not just some. So requiring the use of a non-trivial-to-discover trait to make that functionality available does not make sense to me. 👉 Did that in 2311bcf4223ccb8011d2569fd67e6a4d141fcb5a through e2c02c4963a90d618b8606313585c3122709ffc8.

Given that there's now an initial MR, marking as needing review 😊

Given that 📌 Change 'ignore'/'ignored' to 'exclude'/'excluded' for consistent naming in public API: CollectIgnoredPathsEvent vs getExcludedPaths() Fixed has now also landed, I think we should be getting this done soon 😊

Needs rebase for the past month of changes 😅 Any takers?

Rebased.

The tests are only failing due to the weird mockery going on in \Drupal\Tests\package_manager\Kernel\PathExcluder\SqliteDatabaseExcluderTest::mockDatabase(). We're simulating ::getConnectionOptions() to return something other than what the real one would, but that's somehow failing.

Will need to investigate the root cause.

But the overall proposal is still solid. So marking as needing review.

Tests not passing...?

Yes, tests are not passing. I've already spent 30 mins banging my head against the wall. Still want review on the overall approach.

Perhaps your different perspective spots the problem right away? 😊

Now tests should pass!

Updated issue summary per #10 and #12.

Now the amount of remaining work becomes more clear. Still, keeping as needing review, for:

1. CollectIgnoredPathsEvent absorbing functionality owned by PathExclusionsTrait in HEAD, which I believe is a bad API surface to support
2. the list of remaining work

👆 both of those need review.

I think removing PathExclusionsTrait makes sense. Crossing that off the remaining tasks list.

Rebasing now that 🐛 str_starts_with($path, '/') does not correctly detect absolute paths on Windows Fixed landed.

I think \Drupal\package_manager\FailureMarker should be internal.

I can't think of any situation where code that isn't ours should be interacting with it. It's only a service because that was a convenient and sensible architectural decision, but it's not intended to be used by external code. It's not documented in package_manager.api.php.

Review of @phenaproxima's proposal/updated MR

whoever heard of abstract final

🤣👏

Diffstat

+172, −260

🤩🧹

→ 🚢 ?

Should \Drupal\automatic_updates\UpdateStage be internal?
\Drupal\automatic_updates\ReleaseChooser

\Drupal\automatic_updates\AutomaticUpdatesServiceProvider

RequestedUpdateValidator

These are mentioned in this issue but I think they should be internal

Otherwise I think it looks good

I agree those should all be internal and, where possible, final.

Excellent catches. Agreed they should all be @internal. The validators and provider class can be final.

I think \Drupal\automatic_updates\Validator\StagedDatabaseUpdateValidator
\Drupal\package_manager\Exception\StageOwnershipException
\Drupal\package_manager\Validator\ComposerValidator
DuplicateInfoFileValidator
\Drupal\package_manager\Validator\StageNotInActiveValidator
\Drupal\package_manager\Validator\SymlinkValidator
can be final

Looks great! thanks

Great to finally land this! I think making almost everything final and internal to begin with is wise, and we can open up the API gradually as needed.

💯

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