Deprecate archiver procedural functions

This looks good, as does the CR. Manually verified all instances of archiver_get_extensions() and archiver_get_archiver are gone.

+++ b/core/lib/Drupal/Core/Archiver/ArchiverManager.php
@@ -61,4 +72,50 @@ public function getInstance(array $options) {
+   *   If no archiever plugin qualifies for the given file.

Misspelt.

+++ b/core/includes/common.inc
@@ -1227,43 +1227,43 @@ function drupal_check_incompatibility($v, $current_version) {
 function archiver_get_archiver($file) {
-  // Archivers can only work on local paths
-  $filepath = \Drupal::service('file_system')->realpath($file);
-  if (!is_file($filepath)) {
-    throw new Exception(t('Archivers can only operate on local files: %file not supported', ['%file' => $file]));
-  }
-  return \Drupal::service('plugin.manager.archiver')->getInstance(['filepath' => $filepath]);
+  @trigger_error("archiver_get_archiver() is deprecated in Drupal 8.7.x and will be removed in Drupal 9.0.x. Use \Drupal::service('plugin.manager.archiver')->getExtensions() instead. See https://www.drupal.org/node/2999951.");
+  return \Drupal::service('plugin.manager.archiver')->getArchiverByFile($file);
 }

I'm not sure that this is worth having in the API. We replace this with \Drupal::service('plugin.manager.archiver')->getInstance(['filepath' => $filepath]) and make it the caller's responsibility to have a real path. Since looking at update_manager_file_get() which provides the path for the only real usage in core I think this will return paths that will work with \Drupal::service('plugin.manager.archiver')->getInstance(['filepath' => $filepath])

There's 8 usages in contrib http://grep.xnddx.ru/search?text=archiver_get_archiver and all of it targeted for local file.
2 of them using to make "realpath" conversion before calling the function.
I think it needs to change docs of getInstance() to point that filepath is required key in configuration to instantiate plugin (probably needs follow-up)

Re-rolled and and trying to implement #12

As getInstance() supposed to work on local files only I'm sure it just needs to be documented somehow

Also it needs deprecation test and update change record

+++ b/core/lib/Drupal/Core/Archiver/ArchiverManager.php
@@ -26,11 +34,14 @@ class ArchiverManager extends DefaultPluginManager {
    */
-  public function __construct(\Traversable $namespaces, CacheBackendInterface $cache_backend, ModuleHandlerInterface $module_handler) {
+  public function __construct(\Traversable $namespaces, CacheBackendInterface $cache_backend, ModuleHandlerInterface $module_handler, FileSystemInterface $file_system) {
     parent::__construct('Plugin/Archiver', $namespaces, $module_handler, 'Drupal\Core\Archiver\ArchiverInterface', 'Drupal\Core\Archiver\Annotation\Archiver');
     $this->alterInfo('archiver_info');
     $this->setCacheBackend($cache_backend, 'archiver_info_plugins');
+    $this->fileSystem = $file_system;

+++ b/core/modules/update/src/Form/UpdateManagerInstall.php
@@ -47,11 +55,14 @@ class UpdateManagerInstall extends FormBase {
    */
-  public function __construct($root, ModuleHandlerInterface $module_handler, $site_path) {
+  public function __construct($root, ModuleHandlerInterface $module_handler, $site_path, ArchiverManager $archiver_manager) {
     $this->root = $root;
     $this->moduleHandler = $module_handler;
     $this->sitePath = $site_path;
+    $this->archiverManager = $archiver_manager;
   }

These need to do the "NULL dance". optional argument, if (!$file_system), then fetch from \Drupal::service() and do a @trigger_error().

Solved "NULL dance" for ArchiverManager::construct()

Upload interdiff

+++ b/core/lib/Drupal/Core/Archiver/ArchiverManager.php
@@ -16,6 +17,13 @@
+  /**
+   * The file system service.
+   *
+   * @var \Drupal\Core\File\FileSystemInterface
+   */
+  protected $fileSystem;

Is this needed? I can't see where it is used.

Note fixed

Add test and clean-up messages

valid path

Fix codestyle & uncomment test

Still can't figure why tests fails

There were 2 failures:

1) Drupal\KernelTests\Core\Common\LegacyFunctionsTest::testArchiverGetExtensions
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 @expectedDeprecation:
-%A  archiver_get_extensions() is deprecated in Drupal 8.7.0 and will be removed in Drupal 9.0.0. Use \Drupal\Core\Archiver\ArchiverManager::getExtensions() instead. See https://www.drupal.org/node/2999951
+  Install profile will be a mandatory parameter in Drupal 9.0.

2) Drupal\KernelTests\Core\Common\LegacyFunctionsTest::testArchiverGetArchiver
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 @expectedDeprecation:
-%A  archiver_get_archiver() is deprecated in Drupal 8.7.0 and will be removed in Drupal 9.0.x. Instead, get plugin.manager.archiver service from container and call getInstance() method on it. For example $archiver->getInstance(["filepath" => $file]); See https://www.drupal.org/node/2999951
+  Install profile will be a mandatory parameter in Drupal 9.0.

sadly missed that

still applies

+++ b/core/includes/common.inc
@@ -1245,37 +1245,43 @@ function drupal_check_incompatibility($v, $current_version) {
+ * @deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Use
...
+ * @deprecated in Drupal 8.7.0 and will be removed before Drupal 9.0.0. Instead,

8.8.x now.

Otherwise LGTM.

Addressed #31

+++ b/core/modules/update/src/Form/UpdateManagerInstall.php
@@ -47,11 +55,14 @@ class UpdateManagerInstall extends FormBase {
+  public function __construct($root, ModuleHandlerInterface $module_handler, $site_path, ArchiverManager $archiver_manager) {

We're doing the change in the constructor in a form. However, forms are internal but in the recent issues I have worked on we preserve the BC so that the contrib that extends it doesn't break. So I looked at its usage http://grep.xnddx.ru/search?text=UpdateManagerInstall&filename= and it's not used in contrib apart from portfolio_theme module which committed the drupal core in the repo. Also, there are few other modules which are altering the module so we're good with respect to this change.

Since we have the create() pattern here, I think it's fine to not do a trigger_error for the additional parameter.

review credits

Committed fc301d8 and pushed to 8.8.x. Thanks!

published change record

Note: y'all broke the Update Manager's ability to install from .zip files with this change.

#3101299: Install module from .zip URL fails

// Archivers can only work on local paths
This comment in archiver_get_archiver() was trying to say that \ZipArchive::open() can't handle file streams at all. We absolutely need the call to realpath(), which is now gone.

I don't understand why the new service doesn't do this for you automatically, and we force the callers to have to worry about this.

But if we're forcing callers to worry about it, we failed to properly do this in update_manager_file_get(). It's still returning paths like:

temporary://update-cache-edd502a3/pathauto-8.x-1.6.zip

I'll be the first to admit that update.module's tests are sorely lacking in all sorts of ways.

Obviously, way too late to revert this now, so I guess we'll just fix it at #3101299 and backport the fix.

I'd greatly appreciate it if any of you who were involved here would be willing to help out over there, especially with input on where/how to add tests for this so we don't break it again.

Thanks!
-Derek

p.s. For now, I edited the CR to match what the final patch here did, and to include the warning about callers needing to use realpath():

https://www.drupal.org/node/2999951/revisions/view/11422137/11767365