Splitting off from #609772: Impossible to extend the FileTransfer class system in contrib which is already a monster...

We need tests that verify it's possible to swap out your own Updater class for the Update manager via contrib. It's possible it works and all we need are tests. However, given that *no one* has tried this yet, it's entirely possible the API is broken in various ways that make it impossible to plug in your own classes for this.

Solving this issue will get us most of the way on #605276: Add tests for the update manager as well (although we should still have more test coverage of the Update manager UI, etc).

Comments

sun.core’s picture

Priority: Major » Normal

At this point, test-only issues are no longer considered major.

Crell’s picture

Assigned: Unassigned » Crell
Status: Active » Needs review
StatusFileSize
new1.78 KB

If I understand how the registration works properly, the attached test should verify that contrib can, in fact, override the class that gets used for updating modules. (Themes are identically handled by the same code so do not need their own test.)

dww, please verify and RTBC.

dww’s picture

Status: Needs review » Needs work
StatusFileSize
new2.24 KB

Thanks for getting this rolling! I fixed some code style problems (whitespace) and comment formatting.

Although this is a good start, I'm concerned about a few things:

A)

  $updater = Updater::factory('modules/system');

The ModuleUpdater classes can't currently deal with core at all. I know all we're testing is if we get an instance of the right class back. But, this seems a bit fragile given that we might want to put some logic in ModuleUpdater that prevents itself from working with a core module at all. We certainly wouldn't want to give anyone the impression that the above code was something they could do. So, either we need a comment here explaining why in this case it's okay, or we should do something else here entirely. However, I'm not exactly sure what that should be, given that simpletest has to be self-contained and not rely on true contribs. So, I guess a comment is the best we can do (unless someone else has a bright idea).

B) This is basically a unit test to see if the class instantiation works inside a full bootstrap. However, what we really need to do is see if the class instantiation works from inside authorize.php. As we saw with #609772: Impossible to extend the FileTransfer class system in contrib even if the class itself "works" and there's a system that's supposed to let you register your own classes, the way that core is actually using that system doesn't work at all. So, while this test is helpful to make sure the info hook basically works at all, we really need to verify that it actually works in practice. Hence, we need a functional test here, not just a unit test. Ideally, we need a fake FileTransfer class and a fake Updater class and a test that actually runs through authorize.php and ensures that our fake Updater class was successfully instantiated even from authorize.php.

cosmicdreams’s picture

Status: Needs work » Needs review

bumping to needs review so that dww's patch can go through the test-bot.

dww’s picture

Status: Needs review » Needs work

Okay, great, the bot is happy (for now). But this issue still needs work for the reasons I listed in #3...

cosmicdreams’s picture

Holding out hope that there will be a patch to test tonight.

Crell’s picture

Assigned: Crell » Unassigned

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.