Move lock backend to PSR-0 code

Thanks! That makes much more sense to me.

Functionally, this patch looks complete and ready to me. However, there are a lot of coding style and language/typo/grammar issues in the phpDoc and inline comments.

+++ b/core/lib/Drupal/Component/Lock/DatabaseLockBackend.php
@@ -0,0 +1,100 @@
+      $success = (bool) db_update('semaphore')

Unfortunately the direct DB function calls are a hard dependency on Drupal, and even on procedural Drupal, so this code cannot go in Component at this point. :-( Hopefully we can do that later, but for a straight simple PSR-0-ification it should go in Drupal\Core.

Cleaned up the code and comments a bit. This looks ready to me :)

+++ b/core/lib/Drupal/Core/Lock/DatabaseLockBackend.php
@@ -0,0 +1,117 @@
+class DatabaseLockBackend extends LockBackendAbstract {

Do we really need the word "Lock" in every class? I don't think so, since we're already in the Lock namespace. It's redundant, like putting Database into the name of every DB class.

(Yeah, sorry for the many one-line reviews. It's late.)

I can see both perspectives, and the arguments for either are sound.

Personally, I can especially relate to @pounard's arguments, as I'm not using an IDE, so it's troublesome to figure out what a class name actually refers to in a 1,000 lines file.

Let me try this:

• Code within a namespace should not have to duplicate its own component name all over the place.
• Code that uses namespaced code should use one of two options to ensure code readability and clarity:
  
  1. Assign a more explicit alias for the used class; e.g.:
     

     use Drupal\Core\Lock\DatabaseBackend as LockDatabaseBackend;
     
       $locker = new LockDatabaseBackend();
     

  2. Do not reference the namespace using use, but reference the full namespace instead; e.g.:
     

       $locker = new Drupal\Core\Lock\DatabaseBackend();
     

I don't think aliases intrinsically are "meant for" clarity or collision. They can work for both.

Our current coding standards say they are strictly for collision avoidance, and how to pick an alias name is very strict as well. That was done to minimize the potential for needless bikeshedding over a name. However, given the potential clarity issues we're running into (eg, this issue), I'm coming around to suggesting sun's option 1 as an allowed alternative to use where it would aid in clarity.

That said, I don't think we should hold up this issue on it. So I'm marking this back up to CNR, and let's open a new issue to discuss loosening the namespace rules. We can always rename these classes later if needs be.

This looks good to me, we can remove the legacy procedural wrappers in #1225404: Modern event based lock framework proposal, I'll let someone else mark it RTBC though.

+++ b/core/includes/lock.inc
@@ -98,89 +97,11 @@ function _lock_id() {
+function lock_acquire($name, $timeout = 30.0) {
+  return lock()->acquire($name, $timeout);
 }

Do we need to keep all of the wrapper functions around? It's really not any harder for people to call lock()->acquire() than lock_acquire() (and similar for the other wrapper functions), and that would be more consistent with the cache and queue systems.

Let's just cut to the chase.

13 days to next Drupal core point release.

re #12 - #14: Actually, there's a 3rd option, which kinda replaces the 1st and is much much cleaner:

3. Assign an explicit alias for the imported namespace:
   

   use Drupal\Core\Lock as Lock;
   
     $locker = new Lock\DatabaseBackend();
   

See PHP manual on Using namespaces

sun: Let's make a new issue, please.

I think #18 is the best way, but it can be even simpler as it is not an alias. as Lock is unnecessary.

use Drupal\Core\Lock;

$locker = new Lock\DatabaseBackend();

#17 still needs to be addressed either way.

jbrown: *Please* do not restart a coding standards discussion in this thread. That is the most effective way to destroy any progress it could have. That should be done elsewhere.

I'm ok keeping a bc layer in this patch, we did the same with the cache api changes when moving away from wrappers there. But we should have an explicit follow-up to remove it - can be a novice task since it's find/replace mostly.

#8: 1477446-api_compatible_PSR-0_lock_backend-component-8.patch queued for re-testing.

I recommend we proceed with the patch as is if the shortname is the only issue, and revise later after #1507828: [policy, no patch] Revised standards for class naming within namespaces since that will likely affect a number of subsystems.

+++ b/core/includes/install.core.incundefined
@@ -298,6 +298,16 @@ function install_begin_request(&$install_state) {
+  // The install process cannot use the database lock backend since the database
+  // is not fully up, so we use a null backend implementation durting the
+  // installation process. This will also speed up the installation process.
+  // The site being installed will use the real lock backend when doing AJAX
+  // requests but, except for a WSOD, there is no chance for a a lock to stall
+  // (as opposed to the cache backend) so we can afford having a null
+  // implementation here.
+  require_once DRUPAL_ROOT . '/core/includes/lock.inc';

Might be out of scope for this issue, but this, and the lock() function, could benefit from #1497230: Use Dependency Injection to handle object definitions :

// Register the lock backend.
drupal_container()->register('lock_backend', 'Drupal\Core\Lock\DatabaseLockBackend');

// Retrieve the lock backend.
$lock_backend = drupal_container()->get('lock_backend');

You can also still swap out which lock system it uses:

// Swap out the lock backend with MyModule's lock backend.
drupal_container()->register('lock_backend', 'Drupal\mymodule\LockBackend');

+++ b/core/includes/lock.incundefined
@@ -53,39 +53,38 @@
-    $lock_id = uniqid(mt_rand(), TRUE);
-    // We only register a shutdown function if a lock is used.
-    drupal_register_shutdown_function('lock_release_all', $lock_id);
+    drupal_register_shutdown_function(array($lock_backend, 'releaseAll'));

Do we really need to manually call releaseAll() here? Could we just use a Destructor in the DatabaseLockBackend class? I'm not entirely sure if PHP uses them correctly though.

1 days to next Drupal core point release.

Didn't mean to change status.

I mean, something like this instead of calling drupal_register_shutdown_function():

class DatabaseLockBackend {
  function __destruct() {
    $this->releaseAll();
  }
}

Either way, that could be a follow up. I definitely see this as WAY better than what was in there before. It's a port to PSR-0, and I definitely see it improving once we get the other systems in place too. Great work, guys. I'm being aggressive with this and setting it to RTBC.

Looks good, I've committed/pushed this to 8.x, re-opened #1225404: Modern event based lock framework proposal as well.

This is an API change for lock backends so marking as such.

Quick follow-up for two little doc problems that were introduced with this patch. Not marking "needs review", because the change notification is probably of more concern.

Change notification written, revisions welcome.

Should we create a follow-up issue to change core to use the new methods?

That change notification looks complete to me. Restoring previous settings (but leaving at NR for the followup in #36). Thanks @Cottser!

We should probably also open followups for converting core to the new methods where possible as Cottser suggests, and for the issues mentioned in #29 - #32.

I think #36 is RTBC actually.

Re #40: Ahh, that makes sense and would also make for a great inline comment! :)

Committed the follow-up.