Skip to content
Snippets Groups Projects

Decorator pattern

Open Brad Jones requested to merge issue/drupal-2347867:2347867-race-conditions-with into 11.x

Closes #2347867

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
125 125 *
126 126 * The general format for the $databases array is as follows:
127 127 * @code
128 * $databases['default']['default'] = $info_array;
129 * $databases['default']['replica'][] = $info_array;
130 * $databases['default']['replica'][] = $info_array;
131 * $databases['extra']['default'] = $info_array;
128 * $databases[\Drupal\Core\Database\Database::DEFAULT_KEY][\Drupal\Core\Database\Database::DEFAULT_TARGET] = $info_array;
129 * $databases[\Drupal\Core\Database\Database::DEFAULT_KEY][\Drupal\Core\Database\Database::REPLICA_TARGET][] = $info_array;
130 * $databases[\Drupal\Core\Database\Database::DEFAULT_KEY][\Drupal\Core\Database\Database::REPLICA_TARGET][] = $info_array;
131 * $databases['extra'][\Drupal\Core\Database\Database::DEFAULT_TARGET] = $info_array;
  • Comment on lines +128 to +131

    Is it really increasing readibility?

  • The FQCN is not super readable, no, but this is also a sample code snippet that kinda needs to exist on its own. I don't feel strongly about this here, more so in other contexts where you for instance are using default as both a key and a target in the same code snippet. I think we could keep this file as-is while still using the constant where it helps improve readability and you can import the shorter alias.

  • I tried to use this pattern in a contrib module recently. Regrettably module code is not loaded at the point in time when the settings file is loaded. I think that more people will try to start using this pattern if core is introducing constants as array keys in settings.php. And consequently more people will fail and will have to debug the same problem as myself. For this reason, I'd prefer if we'd stick to string keys in settings.php.

  • Yeah, I'm getting enough push-back on this that I think it'll have to get backed out. I generally disagree in the case of core, but it's not the main thrust of the issue/MR and so not worth bikeshedding. Agree that it would be difficult for contrib to do the same. I would love to move most all of this configuration out of settings and into DI parameters but that's still a way off.

  • Brad Jones changed this line in version 48 of the diff

    changed this line in version 48 of the diff

  • Please register or sign in to reply
  • 1 <?php
    2
    3 declare(strict_types=1);
    4
    5 namespace Drupal\Core\Database\Exception;
    6
    7 use Drupal\Core\Database\DatabaseException;
    8
    9 /**
    10 * Exception thrown when a connection does not implement transactions.
    11 */
    12 final class TransactionsNotSupportedException extends \RuntimeException implements DatabaseException {}
  • 2
    3 declare(strict_types=1);
    4
    5 namespace Drupal\Core\Database;
    6
    7 use Drupal\Core\Database\Event\DatabaseEvent;
    8 use Drupal\Core\Database\Exception\TransactionsNotSupportedException;
    9 use Drupal\Core\Database\Transaction\TransactionManagerInterface;
    10 use Drupal\Core\Pager\PagerManagerInterface;
    11
    12 /**
    13 * A decorator class wrapping a connection, not supporting transactions.
    14 *
    15 * @internal
    16 */
    17 final class NonTransactionalConnection extends Connection {
    • Thinking aloud, I'm not sure it's feasible. But: if we do not extend Connection, then maybe we do not need all the boilerplate and just use a __call magic method to passthrough to the wrapped connection methods. So also non-standard methods in custom drivers could remain accessible?

    • I wish. The problem is that Connection is an abstract class that basically acts as an interface. So there are Connection typehints everywhere, and there is no underlying interface. Otherwise yes this would be as simple as a __call() implementation. Alas, no can do if we choose to use a decorator.

    • Agh. How about using an union type Connection|NonTransactionalConnection in the places where we could expect one or the other? Is that a BC concern?

    • I think we could do that in some places around core but it doesn't solve the underlying problem that we can't control extending code expecting only a Connection. The better solution would be to typehint on an interface, but again, that's a bigger lift and would be a separate issue. The problem here is, I think, that the original authors didn't anticipate something like this and so an interface wasn't in the design.

    • Brad Jones changed this line in version 30 of the diff

      changed this line in version 30 of the diff

    • Please register or sign in to reply
  • 32 33 protected Connection $connection,
    33 34 protected TimeInterface $time,
    34 35 ) {
    36 if (!($connection instanceof NonTransactionalConnection)) {
    37 @trigger_error('Calling ' . __METHOD__ . '() without an explicitly non-transactional database connection is deprecated in drupal:10.4.0 and will be required in drupal:12.0.0. See https://www.drupal.org/node/3310017', E_USER_DEPRECATED);
  • Brad Jones added 1 commit

    added 1 commit

    • 88f14090 - Apply 1 suggestion(s) to 1 file(s)

    Compare with previous version

  • Brad Jones added 126 commits

    added 126 commits

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    • b5f6354e - Don't use non-transactional service with sqlite

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    • 74fc55bd - Fix tests which install non-active database drivers

    Compare with previous version

  • Brad Jones added 2 commits

    added 2 commits

    • 42a92f2b - Revert "Fix tests which install non-active database drivers"
    • ff7b04b1 - Fix backend override for lazy service

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    • 8107ebb5 - More narrowly tailor backend-overridable tag

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    • c38c4b45 - Handle original public value when registering proxy services

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    • f85242b9 - Replace overridden services, don't alias them.

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    Compare with previous version

  • Brad Jones added 22 commits

    added 22 commits

    • 1cfca5e1...e700aa81 - 3 commits from branch project:11.x
    • e700aa81...c6500891 - 9 earlier commits
    • 0f2e0c7b - Delete duplicate test
    • 6cf654f8 - Don't use non-transactional service with sqlite
    • 75117123 - Fix tests which install non-active database drivers
    • 938f19c1 - Revert "Fix tests which install non-active database drivers"
    • 32a8f653 - Fix backend override for lazy service
    • aab5b6a1 - More narrowly tailor backend-overridable tag
    • da11f437 - Handle original public value when registering proxy services
    • e8bf1b27 - Replace overridden services, don't alias them.
    • e8580b69 - Fix CS
    • 0958ecb0 - Include now-legacy check for existing aliases.

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    • a080cabe - Make sure replaced service is still lazy and fix comment

    Compare with previous version

  • Brad Jones added 32 commits

    added 32 commits

    • a080cabe...9f3ee1ff - 11 commits from branch project:11.x
    • 9f3ee1ff...3af7008b - 11 earlier commits
    • 8d63a81f - Fix tests which install non-active database drivers
    • d320f622 - Revert "Fix tests which install non-active database drivers"
    • 3d52b188 - Fix backend override for lazy service
    • 63b2e680 - More narrowly tailor backend-overridable tag
    • a4628da3 - Handle original public value when registering proxy services
    • 43770eec - Replace overridden services, don't alias them.
    • d1bd195b - Fix CS
    • 8dce5054 - Include now-legacy check for existing aliases.
    • 3027483a - Make sure replaced service is still lazy and fix comment
    • 732661a0 - Move backend-swap pass to be first

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    • 95e27351 - Clone definitions while overriding

    Compare with previous version

  • Brad Jones added 24 commits

    added 24 commits

    • c1708b32 - 1 commit from branch project:11.x
    • c1708b32...2b0d91dc - 13 earlier commits
    • 3b997189 - Fix backend override for lazy service
    • bda5b722 - More narrowly tailor backend-overridable tag
    • ec5aa249 - Handle original public value when registering proxy services
    • 56587157 - Replace overridden services, don't alias them.
    • 725a8d63 - Fix CS
    • e9d88078 - Include now-legacy check for existing aliases.
    • f8412e69 - Make sure replaced service is still lazy and fix comment
    • e6eb7f9d - Move backend-swap pass to be first
    • da306ae1 - Clone definitions while overriding
    • 2212a137 - Fix override-check test

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    • 2a875ed4 - Ensure locations of db-driver backend overrides are consistent

    Compare with previous version

  • Brad Jones added 28 commits

    added 28 commits

    • 2a875ed4...5a3ca127 - 3 commits from branch project:11.x
    • 5a3ca127...38dba452 - 15 earlier commits
    • 0d9f51f9 - Handle original public value when registering proxy services
    • c62b1fe6 - Replace overridden services, don't alias them.
    • 95398a2d - Fix CS
    • dbc4e0b0 - Include now-legacy check for existing aliases.
    • 1cbeb219 - Make sure replaced service is still lazy and fix comment
    • 9ef1e50c - Move backend-swap pass to be first
    • a0a2993a - Clone definitions while overriding
    • 1caa2ee2 - Fix override-check test
    • bc9b3f21 - Fix PHPCS
    • f705fbcf - Ensure locations of db-driver backend overrides are consistent

    Compare with previous version

  • Brad Jones added 459 commits

    added 459 commits

    • f705fbcf...49f6e6bb - 434 commits from branch project:11.x
    • 49f6e6bb...a6e460c1 - 15 earlier commits
    • 2c3c0aa9 - Handle original public value when registering proxy services
    • 84ccb7a5 - Replace overridden services, don't alias them.
    • be12f6a6 - Fix CS
    • 46a0515d - Include now-legacy check for existing aliases.
    • 05c7c250 - Make sure replaced service is still lazy and fix comment
    • e673a096 - Move backend-swap pass to be first
    • 5c3d66ad - Clone definitions while overriding
    • 04819a8e - Fix override-check test
    • 3d4d4f36 - Fix PHPCS
    • 2a7e4657 - Ensure locations of db-driver backend overrides are consistent

    Compare with previous version

  • Brad Jones added 44 commits

    added 44 commits

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    • 4b4b0db9 - Try out new connection interface

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    • 525079e3 - Add to phpstan baseline given new interface

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    • 53c0cdda - Adjust typehint in the persistent db lock backend.

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    • a08ecf33 - Additional typehint updates for non-transactional services

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    • f2763434 - Remove overzealous deprecation notice

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    • dac5e1e3 - Fix tests with unnecessary container overrides

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    • 5b35ae5a - Handle more hard-coded tests

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    • 323758e3 - Add method to flag transactions support

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    • 8bab0e94 - Add method to flag transactions support

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    Compare with previous version

  • Brad Jones added 342 commits

    added 342 commits

    Compare with previous version

  • Brad Jones added 1 commit

    added 1 commit

    Compare with previous version

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading