Plugins, annotations, PluginManagers, oh my!

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

Eclipse

xjm’s picture

archiver_info_shiny_edition.patch queued for re-testing.

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

Bit of nitpicking, my apologies. Functionally this is RTBC, the conversion looks awesome.

+++ b/core/includes/common.incundefined
@@ -6551,24 +6525,11 @@ function archiver_get_extensions() {
-  $filepath = drupal_realpath($file);
-  if (!is_file($filepath)) {
+  $filename = drupal_realpath($file);
+  if (!is_file($filename)) {

Here you change the variable name away from $filepath, but both Components refer to it as such. Why not leave it?

+++ b/core/lib/Drupal/Core/Archiver/ArchiverManager.phpundefined
@@ -0,0 +1,58 @@
+<?php
+
+/**
+ * Definition of \Drupal\Core\Archiver\ArchiverManager.
+ */
+
+namespace Drupal\Core\Archiver;
+
+
+use Drupal\Component\Plugin\Factory\DefaultFactory;

Contains \Drupal\...

and there is an extra blank line

+++ b/core/lib/Drupal/Core/Archiver/ArchiverManager.phpundefined
@@ -0,0 +1,58 @@
+use Drupal\Core\Plugin\Discovery\CacheDecorator;
+
+class ArchiverManager extends PluginManagerBase {

Missing a docblock

+++ b/core/lib/Drupal/Core/Archiver/ArchiverManager.phpundefined
@@ -0,0 +1,58 @@
+   *   An array of paths keyed by it's corresponding namespaces.

s/it's/its

+++ b/core/lib/Drupal/Core/Archiver/ArchiverManager.phpundefined
@@ -0,0 +1,58 @@
+   * Overrides \Drupal\Component\Plugin\PluginManagerBase\createInstance().

::createInstance, not \

+++ b/core/lib/Drupal/Core/Archiver/ArchiverManager.phpundefined
@@ -0,0 +1,58 @@
+   * Implements \Drupal\Core\PluginManagerInterface\getInstance.

::getInstance()

+++ b/core/modules/system/lib/Drupal/system/Plugin/Core/Archiver/Tar.phpundefined
@@ -0,0 +1,25 @@
+ * Definition of Drupal\system\Plugin\Core\Archiver\Tar.

+++ b/core/modules/system/lib/Drupal/system/Plugin/Core/Archiver/Zip.phpundefined
@@ -0,0 +1,27 @@
+ * Definition of Drupal\system\Plugin\Core\Archiver\Zip.

Contains \Drupal\

chx’s picture

Status: Needs work » Needs review
FileSize
3.17 KB
10.47 KB
tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

catch’s picture

#4: 1950726_4.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1950726_4.patch, failed testing.

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.31 KB
10.19 KB

Keeping up with HEAD.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

OKie doke!

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

tim.plunkett’s picture

Title: Convert hook_archiver_info into the New Shiny(TM) » Change notice: Convert hook_archiver_info into the New Shiny(TM)
Priority: Normal » Critical
Status: Closed (fixed) » Active

This needs a change notice. Also see #1987298: Shorten directory structure and PSR-0 namespacing for plugins for the updated path.

chx’s picture

Assigned: chx » Unassigned

I am not writing this right now

nielsonm’s picture

I created the change record. https://drupal.org/node/2003376

jibran’s picture

Status: Active » Needs work
Issue tags: +Needs change record

Some before and after will certainly help so please add that and name spaces as well. Thanks for the change notice.

chx’s picture

Status: Needs work » Fixed

I took the notice and https://drupal.org/node/1993056, merged them and this is now done.

jibran’s picture

Priority: Critical » Normal
Issue tags: -Needs change record

Thanks @chx.

jibran’s picture

Title: Change notice: Convert hook_archiver_info into the New Shiny(TM) » Convert hook_archiver_info into the New Shiny(TM)

Reverting title.

Status: Fixed » Closed (fixed)

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