Make file usage storage pluggable

Yes. Reviewing the API it is not a normal CRUD that can be converted to an Entity. It's not fieldable, translateable, adding fields sounds like madness, we do not want to get or set invidual properties, it's definitely not one.

I agree that a plugin seems like the sanest route here. File usages are not entities as they have no individual properties, they're just a mapping between an entity and the file (which is also an entity, though not fieldable yet). The retrieval mechanisms in the file usage table typically simple.

I think we should just register the service into DIC in CoreBundle and then use drupal_container()->get('file.usage') much like config() does. No need for a static or another way to plug things. Altering the DIC should be possible no matter what.

Perhaps add that here so if someone does a git blame they find it here. I know how to change the bootstrap DIC but not the proper one. I only mentioned it vaguely that it should be possible; if it already is then it's great.

Thanks that's even so much better! And a DIC change demo, I love it.

About the naming of the class you might want to note #1798420: Document and consider renaming AbstractStorage and what @tim.plunkett wrote about no other classes starting ith Abstract. Other than that, this implemention is personally quite educational for me.

chx asked me to have a look here for Container pedantry...

+++ b/core/lib/Drupal/Core/FileUsage/DatabaseFileUsageBackend.php
@@ -0,0 +1,86 @@
+    db_merge('file_usage')
+      ->key(array(
+        'fid' => $file->fid,
+        'module' => $module,
+        'type' => $type,
+        'id' => $id,
+      ))
+      ->fields(array('count' => $count))
+      ->expression('count', 'count + :count', array(':count' => $count))
+      ->execute();

Since this object is in the DIC, it should take the Database connection as a dependency in its constructor. Then db_merge() gets replaced with $this->connection->merge(), (and the same for the other db_* functions).

To do so, modify the register call to chain the following after the register method:

->addArgument(new Reference('database'));

And then add a constructor that takes a single parameter, a DB connection, and assign it to $this->connection. (Or $this->dbConnection, or whatever. The name doesn't matter except as a convention.)

+++ b/core/lib/Drupal/Core/FileUsage/DatabaseFileUsageBackend.php
@@ -0,0 +1,86 @@
+  function delete(File $file, $module, $type = NULL, $id = NULL, $count = 1) {

All methods should define their public/protected state explicitly.

+++ b/core/lib/Drupal/Core/FileUsage/FileUsageInterface.php
@@ -0,0 +1,72 @@
+  public function list_usage(File $file);

Method names should always be lowerCamel case. That is, listUsage().

Edit: The reason to make the DB connection injected is that it's probably possible to then convert the tests from WebTest to UnitTest (unless there's other file system dependencies I'm not recognizing), which should improve their accuracy and performance about 10000%. :-)

Crell's concerns are addressed. interdiff between #13 and #17 is attached.

+++ b/core/lib/Drupal/Core/FileUsage/DatabaseFileUsageBackend.php
@@ -0,0 +1,100 @@
+  /**
+   * Construct the DatabaseFileUsageBackend.
+   *
+   * @param Drupal\Core\Database\Connection $connection
+   *   The database connection which will be used to store the route
+   *   information.
+   * @param string $table
+   *   (optional) The table to store the route info in. Defaults to 'router'.
+   */
+  public function __construct(Connection $connection) {
+    $this->connection = $connection;
+  }

Oops, left in the router docblock parameters. :-) I'd actually fix this by specifying the table name, too, and using that as a protected property like $connection, so that the docblock is accurate, rather than removing the @param. (Although the @param needs to be updated with what the actual default value is, since I assume it's not 'router'.)

Other than that, this looks done to me.

This not a plugin (and I doubt it needs to be one).

OK, Crell's latest concerns are addressed :)

I see that this has already been RTBC'd. Thus, I am not going to change the status.

However, I would like to plead going forward that we add type hinting to all @param and @return directives in docblocks, particularly in any interface files that added or modified. It really helps make the docs API website much clearer when someone down the road goes to use this spiffy new feature.

I noticed that the patch in #23 needed a couple of other documentation fixes. Attached is an untested patch and interdiff that rewraps a couple of doc lines, adds a couple of default values to docs and makes a stab at adding type hinting to @param and @return directives.

I would appreciate a review to make sure the type hinting is correct.

+++ b/core/lib/Drupal/Core/FileUsage/DatabaseFileUsageBackend.php
@@ -0,0 +1,116 @@
+use Drupal\file\File as File;

The "as" part here is redundant. I won't block the patch on it but just an FYI.

We're back to light green. Let's make it dark green, k? :-)

Why are the classes added to Core when the managed file API is owned entirely by file module now?

Sounds like another round :/

That seems to address the sole concern anyone had with the patch.

That seems to address the sole concern anyone had with the patch.

The only concern I see is registering the class name 'FileBundle' when bundle is an entity term, but it shouldn't hold this up. I just found it odd. Overall this looks good and makes a lot of sense.

FileBundle is a bundle class in the Symfony sense, vis, thing that registers services. Entity "bundles" are getting renamed to subtypes to avoid confusion: #1380720: Rename entity "bundle" to "subtype" in the UI text and help

This looks like sane clean-up to me, and looks like catch's feedback has been addressed. Also has sign-off from at least one of the people who deal with files a bunch in contrib.

However! It no longer applies. :( Can we get a quick re-roll?

Actually it's the same patch that was RTBC before, it applied with fuzz.

Committed and pushed to 8.x. Thanks!

We'll need a change notice for this.

http://drupal.org/node/1812240