Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Preceding #1225404: Modern event based lock framework proposal which has been a giant troll this issue aims to open the discussion of the clear port of the raw lock backend driver, without any API modification, and ensuring changes to the strict minimum. Patch will come in a few minutes.
Comment | File | Size | Author |
---|---|---|---|
#36 | 1477446-clean-up.patch | 1.49 KB | tstoeckler |
#8 | 1477446-api_compatible_PSR-0_lock_backend-component-8.patch | 22.57 KB | amateescu |
#8 | interdiff-7.txt | 10.97 KB | amateescu |
#7 | 1477446-api_compatible_PSR-0_lock_backend-component-2.patch | 20.74 KB | pounard |
#2 | 1477446-api_compatible_PSR-0_lock_backend-1.patch | 20.84 KB | pounard |
Comments
Comment #1
pounardOk here is the first patch.
Tests are OK on my development box.
That is all, pretty much straightforward.
Comment #2
pounardFollowing @amateescu's review, here is a patch with some use statements pointing to non existing classes removed.
Comment #3
sunThanks! 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.
Comment #4
pounardI'm not a native english speaker, don't surprise me! Let's fix those and get this commited so it unblock some other dicussions.
Comment #5
Crell CreditAttribution: Crell commentedUnfortunately 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.
Comment #6
pounardRight, that's exactly why I'd prefer to see DbTng in components.
EDIT: Will fix this patch tonight if no one does meanwhile (note that this is not a hard dependency since the backend can be switched over).
Comment #7
pounardHere it is
Comment #8
amateescu CreditAttribution: amateescu commentedCleaned up the code and comments a bit. This looks ready to me :)
Comment #9
Crell CreditAttribution: Crell commentedDo 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.)
Comment #10
pounardI think we must, because we use any of those classes outside of the Lock namespace, we'd end up with only "DatabaseBackend" for example, which would be redundant, and a potential conflict with other backends from other namespaces.
Namespaces allows us to use longer class names than usual, by drastically reducing conflicts thanks to encapsulation. With the help of the use statement -which is nothing more than a shortcut- let's use that! I don't see why we should shorten names to the point where they doesn't mean anything anymore outside of their context.
The existence of the "as" statement that allows class name aliasing and conflict resolving for such use cases is not an excuse to make our class names implicit: explicit is better, and "DatabaseLockBackend" isn't really long, really.
EDIT:
DatabaseBackend doesn't mean anything and will bring conflicts inside the Drupal context
DatabaseLockBackend is explicit enough to be used in Drupal context everywhere without the need of aliasing
DrupalDatabaseLockBackend is too long, but thanks to namespaces, we can use DatabaseLockBackend, without carrying about the namespace, but still using an explicit class name
Comment #11
pounardNote that the namespace is not part of the classname, internally for PHP it is, but for the end developper, it isn't. In that regard, having the "Lock" word in the class name is not a redundancy, it's is making the class name explicit. I strongly suggest to keep the class names explicit, I hope we're not talking about removing 4 letters because it's 4 letters, this is false purity concern, having an explicit class name is a real concern for users.
Comment #12
sunI 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:
use
, but reference the full namespace instead; e.g.:Comment #13
pounardI'm not sure aliasing is actually the best answer, it's more like a failsafe for a few conflicting cases. Using aliases is the most confusing thing IMHO (some other languages actually solve this by forcing the developer to use fully qualified names in case of ambiguity) If we look at frameworks such as Symfony or Zend, they actually often do repeat the namespace bit, not always thought, depends on the cases I guess.
Comment #14
Crell CreditAttribution: Crell commentedI 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.
Comment #15
pounardFine by me
Comment #16
catchThis 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.
Comment #17
Crell CreditAttribution: Crell commentedDo 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.
Comment #18
sunre #12 - #14: Actually, there's a 3rd option, which kinda replaces the 1st and is much much cleaner:
See PHP manual on Using namespaces
Comment #19
Crell CreditAttribution: Crell commentedsun: Let's make a new issue, please.
Comment #20
jbrown CreditAttribution: jbrown commentedI think #18 is the best way, but it can be even simpler as it is not an alias.
as Lock
is unnecessary.Comment #21
pounardI think using relative names sounds as bad as aliasing, if we start extending these as code standards or implicit standard among the full Drupal core, it will end up such as mess, non grepable and harder to search in it and it will probably drive us insane. What is wrong about explicit class names?
I'm tempted to RTBC it but as I did myself the patch, I'd like someone else to do it.
Please remember that this is a first step, and many other issues will continue to flow arround PSR-0 and OOP core APIs.
Comment #22
Crell CreditAttribution: Crell commented#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.
Comment #23
pounard#17 can be addressed in a later patch. This one is actually backward compatible so let's keep it that way.
Comment #24
catchI'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.
Comment #25
pounardYes, it is.
Comment #26
pounardAnyone for reviewing and RTBC-ing?
Comment #27
RobLoach#8: 1477446-api_compatible_PSR-0_lock_backend-component-8.patch queued for re-testing.
Comment #28
Crell CreditAttribution: Crell commentedI 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.
Comment #29
RobLoachMight be out of scope for this issue, but this, and the
lock()
function, could benefit from #1497230: Use Dependency Injection to handle object definitions :You can also still swap out which lock system it uses:
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.
Comment #30
RobLoachDidn't mean to change status.
Comment #31
pounard@Rob Loach, definitely agree, it was one of my main concern when I started the other issue, now I'm just porting with backward compat in order to make people happy, but I have deep changes proposal waiting this patch to land first.
EDIT: Calling manually a destructor is bad idea, in PHP it will work seamlessly, but calling a destructor has a specific meaning in object oriented code: it must be done by the garbage collector itself, never manually.
Re EDIT: for the record, the drupal shutdown registration ensures this mehtod is being called before the real PHP shutdown, and ensure other objects have not been destructed (thinking of database handle here). If lock doesn't use database, same statement for whatever is the backend. If we simply use destructors, PHP will destruct our objects after it closed resources and probably some other dependency objects before ours (we cannot control that).
Comment #32
RobLoachI mean, something like this instead of calling
drupal_register_shutdown_function()
:Comment #33
RobLoachEither 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.
Comment #34
pounardThank you very much!
Comment #35
catchLooks 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.
Comment #36
tstoecklerQuick 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.
Comment #37
star-szrChange notification written, revisions welcome.
Should we create a follow-up issue to change core to use the new methods?
Comment #38
xjmThat 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.
Comment #39
xjmI think #36 is RTBC actually.
Comment #40
pounardThis is not into the object destructor for a good reason: the shutdown handler must be run before PHP native shutdown handler otherwise some dependencies such as database may be closed before. This would either crash or make a second database connection re-open, both are not a good idea. We could maybe implement the destructor just in case the object ends up being destructed before PHP shutdown, but we cannot rely on it for real shutdown and must continue to use drupal custom one.
Comment #41
tstoecklerRe #40: Ahh, that makes sense and would also make for a great inline comment! :)
Comment #42
catchCommitted the follow-up.