[Order][Sequence] Remove SequenceBundle and move logic of generating numbers to OrderBundle #5505
Conversation
GSadee
commented
Jul 13, 2016
GSadee
commented
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | yes |
| BC breaks? | yes |
| Related tickets | |
| License | MIT |
pjedrzejewski
added
the
BC Break
| /** | ||
| * @author Grzegorz Sadowski <grzegorz.sadowski@lakion.com> | ||
| */ | ||
| class OrderNumberGenerator implements OrderNumberGeneratorInterface |
There was a problem hiding this comment.
final class SequentialOrderNumberGenerator?
There was a problem hiding this comment.
Anyway, this class is not only a generator, it is an assigner also. I would suggest to split this logic, and create a generator, which will only generate number and increment a sequence and a second one, which will check if a number generation is require and if so, just call a generate method. Checker could be also extracted, but I'm afraid that it would be an overengineering.
There was a problem hiding this comment.
Not a bad idea 👍
I have extracted OrderNumberAssigner from this generator
There was a problem hiding this comment.
We can still rename it to SequentialOrderNumberGenerator :)
There was a problem hiding this comment.
And we can still make it final :)
GSadee
changed the title
GSadee
force-pushed
the
remove-sequence
branch
3 times, most recently
from
d086189
to
fd65005
Compare
GSadee
changed the title
| $this->addSql('CREATE TABLE sylius_sequence (id INT AUTO_INCREMENT NOT NULL, idx INT NOT NULL, type VARCHAR(255) NOT NULL COLLATE utf8_unicode_ci, UNIQUE INDEX UNIQ_AD3D8CC48CDE5729 (type), PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB'); | ||
| $this->addSql('DROP TABLE sylius_order_sequence'); | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't know is it needed to generate migration, as for now Sylius does not support/use them (and they probably will be added as one enormous migration in the nearest future).
There was a problem hiding this comment.
There's nothing wrong in generating a migration right now, it may help to jump from 0.19 to 1.0.0-alpha.
| <?php | ||
|
|
||
| /* | ||
| * This file is part of the Sylius package. |
GSadee
force-pushed
the
remove-sequence
branch
4 times, most recently
from
c908fba
to
0c038e9
Compare
| /** | ||
| * @author Grzegorz Sadowski <grzegorz.sadowski@lakion.com> | ||
| */ | ||
| interface SequentialOrderNumberGeneratorInterface |
There was a problem hiding this comment.
This interface should be SequentialOrderNumberGeneratorInterface, Sequential is specific implementation.
|
Thanks Grzesiu, really nice work, please apply @michalmarcinkowski's comment in separate PR. 👍 |
[Order][Sequence] Remove SequenceBundle and move logic of generating numbers to OrderBundle
[Order][Sequence] Remove SequenceBundle and move logic of generating numbers to OrderBundle
