[Theme] Make themes persistable & huuuge refactoring

[pamil]

Q	A
Bug fix?	yes
New feature?	yes
BC breaks?	yes
Deprecations?	no
Fixed tickets	-
License	MIT
Doc PR	-

• Allows to easily mock available themes in functional tests
• No longer requires to clear cache if adding / removing / modifying theme

Unfortunately I had to do some refactoring, so the diff might be a little biased, but it's the last time as v0.17 is coming.

Based on #4287.

[aramalipoor]

What about cache/templates.php? We need to clear/warm it up, to be able to actually use a theme, right?

Another question is about repository, who is responsible to fill it up with available themes? ThemeLocator or just like other resources (fixtures, controllers, etc.)?

[pamil]

Hello Aram!

In fact, currently only theme repository and translation files are set during container compilation. Templates can be loaded without being warmed up first (but it will be needed for production), translations sources are required to be added by compiler pass (for now), assets need to be dumped first (but can be also managed by some kind of a frontend controller in the future).

The themes will be synchronized by a command (I'm just writing it) that runs ThemeSychronizer included in this PR. There won't be any possibility to modify these entities in the backend, but we can use the built-in forms (like choice type) instead of our own. Moreover, we will be able to have valid relations based on foreign keys, not just strings as theme names.

[liverbool]

why you breaks 2.8 (composer.json => "symfony/symfony": "^2.7.7, <2.8")?

[pamil]

@liverbool Symfony 2.8 caused some issues and I'd rather fix it in another PR as this is getting too big (also that requirement is not a part of this one).

[takeit]

Hi @pamil,

if I understood correctly, every time I will want to add a new theme I will need to run ThemeSychronizerCommand and you won't create any logic to upload the themes etc. right ? Don't you plan to add some events, like: sylius.theme.added, sylius.theme.removed when once dispatched (let's say from my app's controller), it will synchronize themes automatically?

[Zales0123]

Maybe ThemeSynchronizeCommand? as there is also ThemeDebugCommand etc.

[pamil]

I'm thinking about removing Theme from ThemeDebugCommand, as it only makes the class name longer and does not add much context.

[Zales0123]

Just keep consistent and it will be good 😄

[pamil]

Done, 👍

[pamil]

@takeit for now the only synchronizer job is to update persisted themes with the real state, the usage of events is a really good idea worth implementing, but for sure not in this PR :)

[pjedrzejewski]

Requring this in Sylius sounds like an overkill, I guess I will learn what is needed in the rest of the PR but that's quite big deps to add.

[pamil]

I'll use another hydrator instead of the Doctrine one and it can be removed.

[gabiudrescu]

me happy now 👯

[tristanbes]

great job @pamil

[pjedrzejewski]

Great work @pamil! :D Hopefully this is last PR of this size before 1.0. :) 👍

[ahilles107]

Do you know when https://github.com/Sylius/SyliusThemeBundle repo will be synchronized?

[pjedrzejewski]

@ahilles107 Subtree split is running right now. :)

[ahilles107]

In Readme link to docs is broken: http://sylius.org/en/latest/bundles/SyliusThemeBundle/index.html docs subdomain is missing.

[sstok]

So if you want to use the ThemeBundle you also need to configure the ResourceBundle?

[ahilles107]

after this PR - yes

[sstok]

OK.. That actually makes me sad as it was a killer feature and now I need to configure the ResourceBundle only to use this. I can understand the integration, but please reconsider the coupling to the Sylius system and make it easier for others to use this.

[liverbool]

👍 @sstok
But bigger issue for now (0.18.x-dev) i can't install resource-bundle case of "sylius/resource-bundle": "^0.17", in theme-bundle's composer.json.

[pamil]

@sstok It's quite unfortunate, I'm thinking about making ThemeBundle independent of ResourceBundle and themes persistance, but for now it's required.

@liverbool PR is coming, ^0.17 is equal to >=0.17,<0.18, gonna change it to accept 0.18 too

[pjedrzejewski]

@pamil We should be consistent with other bundles and use 0.17.*, which is equal to what you mentioned above.

[pamil]

@liverbool should be all right now (there may be some delay because of a subtree split).