8000 [Console][Lock] Allow LockableTrait to use custom stores by stixx · Pull Request #31914 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Console][Lock] Allow LockableTrait to use custom stores #31914

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 12 commits into from
Closed

[Console][Lock] Allow LockableTrait to use custom stores #31914

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
38e7a8a
[Lock] Added the ability to set a store in the LockableTrait by using…
May 30, 2019
7b375db
[Lock] Moved command name of lockable to the container register
May 30, 2019
c097950
Register default store as service to be accessible by the AddConsoleC…
Jun 6, 2019
51626d9
Revert "Register default store as service to be accessible by the Add…
Jun 17, 2019
dff0863
[Lock] Replaced method injection in AddConsoleCommandPass to register…
Jun 21, 2019
ac74dde
[Lock] Added license to LockableInterface
Jun 21, 2019
cf89732
[Lock] Added the ability to set a store in the LockableTrait by using…
May 30, 2019
300bcc8
[Lock] Moved command name of lockable to the container register
May 30, 2019
588d6d9
Register default store as service to be accessible by the AddConsoleC…
Jun 6, 2019
9605bdc
Revert "Register default store as service to be accessible by the Add…
Jun 17, 2019
c1f3749
[Lock] Replaced method injection in AddConsoleCommandPass to register…
Jun 21, 2019
bbfb8aa
[Lock] Added license to LockableInterface
Jun 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[Lock] Added the ability to set a store in the LockableTrait by using… 
… the AddConsoleCommandPass
  • Loading branch information
Jelle van Oosterbosch committed Jun 21, 2019
commit 38e7a8a2bc371f8587df1476117fc83ee23176bc
34 changes: 27 additions & 7 deletions src/Symfony/Component/Console/Command/LockableTrait.php
View file
Open in desktop
8000
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,10 @@

use Symfony\Component\Console\Exception\LogicException;
use Symfony\Component\Lock\Factory;
use Symfony\Component\Lock\Lock;
use Symfony\Component\Lock\LockInterface;
use Symfony\Component\Lock\Store\FlockStore;
use Symfony\Component\Lock\Store\SemaphoreStore;
use Symfony\Component\Lock\StoreInterface;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beware, the interface has been renamed in #32198


/**
* Basic lock feature for commands.
Expand All @@ -24,9 +25,16 @@
*/
trait LockableTrait
{
/** @var Lock */
/**
* @var LockInterface
*/
private $lock;

/**
* @var StoreInterface
*/
private $store;

/**
* Locks a command.
*
Expand All @@ -42,13 +50,15 @@ private function lock($name = null, $blocking = false)
throw new LogicException('A lock is already in place.');
}

if (SemaphoreStore::isSupported()) {
$store = new SemaphoreStore();
} else {
$store = new FlockStore();
if (null === $this->store) {
if (SemaphoreStore::isSupported()) {
$this->store = new SemaphoreStore();
} else {
$this->store = new FlockStore();
}
}

$this->lock = (new Factory($store))->createLock($name ?: $this->getName());
$this->lock = (new Factory($this->store))->createLock($name ?: $this->getName());
if (!$this->lock->acquire($blocking)) {
$this->lock = null;

Expand All @@ -68,4 +78,14 @@ private function release()
$this->lock = null;
}
}

/**
* Sets the internal store for the command to be used.
*
* @param StoreInterface $store
*/
public function setStore(StoreInterface $store)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about passing a non-default store as optional 3rd arg in lock()? and avoid this state issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

meaning the user injects his own store if needed

Copy link
Author
@stixx stixx Jun 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's the case then this change wouldn't be needed because you can implement the Symfony\Component\Lock\Factory service in a command by dependency injection to create locks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right ... the user can also set private $store; already.Then, im not sure what we're solving here 🤔

We should favor pure DI / service usage IMHO.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to solve a configurable store that can be used in commands, this can either be done by:

  • The current solution (setStore in LockableTrait);
  • Implementing a default store in Symfony\Component\Console\Command\Command;
  • Writing a LockableCommand class which basically does what LockableTrait does now, so this could mean bye bye for LockableTrait - would also mean more abstraction;

Why?

Because currently LockableTrait is limited to SemaphoreStore and FlockStore. Also you can use dependency injection but that would mean that you have to overwrite the constructor. Last, it's not configurable as in available in a command by default.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use dependency injection but that would mean that you have to overwrite the constructor.

Is that a real problem? IMHO it's the explicit way.. compared to calling an arbitrary setStore method. At least i agree with https://github.com/symfony/symfony/pull/31914/files#r294103123; if we do this we should add a marker interface for it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we should be using a marker interface. I've updated the PR.

{
$this->store = $store;
}
}
7 changes: 7 additions & 0 deletions src/Symfony/Component/Console/DependencyInjection/AddConsoleCommandPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,13 @@ public function process(ContainerBuilder $container)
if ($aliases) {
$definition->addMethodCall('setAliases', [$aliases]);
}

$storeDefinition = 'lock.store';
if ($container->hasDefinition($storeDefinition)) {
if (\method_exists($definition->getClass(), 'setStore')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit unsure about injecting the store based on method existence.
This is about autoconfiguration which usually works on types, and traits are not types. Should we add a marker LockableInterface and use it as a type for ContainerBuilder::registerForAutoconfiguration()?

$definition->addMethodCall('setStore', [$container->getDefinition($storeDefinition)]);
}
}
}

$container
Expand Down
36 changes: 36 additions & 0 deletions src/Symfony/Component/Console/Tests/DependencyInjection/AddConsoleCommandPassTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use PHPUnit\Framework\TestCase;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Command\LockableTrait;
use Symfony\Component\Console\CommandLoader\ContainerCommandLoader;
use Symfony\Component\Console\DependencyInjection\AddConsoleCommandPass;
use Symfony\Component\DependencyInjection\Argument\ServiceClosureArgument;
Expand Down Expand Up @@ -246,6 +247,34 @@ public function testProcessOnChildDefinitionWithoutClass()

$container->compile();
}

public function testProcessRegistersStoreInCommand()
{
$storeClassName = 'Symfony\Component\Lock\StoreInterface';

$container = new ContainerBuilder();
$container
->register('lock.store', $this->createMock($storeClassName))
->setPublic(false)
;
$container
->register('lockable.command', LockableCommand::class)
->setPublic(false)
->addTag('console.command')
;

$pass = new AddConsoleCommandPass();
$pass->process($container);

$command = $container->get('lockable.command');
$this->assertInstanceOf(LockableCommand::class, $command);

$reflection = new \ReflectionClass($command);
$property = $reflection->getProperty('store');
$property->setAccessible(true);
$store = $property->getValue($command);
$this->assertInstanceOf($storeClassName, $store);
}
}

class MyCommand extends Command
Expand All @@ -256,3 +285,10 @@ class NamedCommand extends Command
{
protected static $defaultName = 'default';
}

class LockableCommand extends Command
{
use LockableTrait;

protected static $defaultName = 'lockable';
}
0