Executable tests on Sylius-Standard & remove duplicated configs #5058

pamil · 2016-05-18T11:18:00Z

Q	A
Bug fix?	no
New feature?	no
BC breaks?	yes
Related tickets	-
License	MIT

pjedrzejewski · 2016-05-18T11:43:46Z

General note, I have not reviewed it in full yet: We need to have same structure as Symfony Standard whenever possible, even if this means duplication of some configs. I don't want to write a whole chapter in docs about why configs are splitted and included separately and answering questions: "where is configuration X, in Symfony it is right there!". I'd rather still live with a small duplication.

pamil · 2016-05-18T13:50:21Z

@pjedrzejewski I agree, but there are over 1500 lines of configuration compared to 100 lines of Symfony Standard, so we must think through what we can duplicate and what rather not.

pjedrzejewski · 2016-05-18T13:53:19Z

The Sylius defaults should not be duplicated of course and I believe they could stay in the Sylius[Shop/Admin/Core accordingly]Bundle, just like they are right now. In main config files I would leave the basic Symfony/Doctrine settings you find in all Symfony apps and anything that's cannot be overriden (fos_rest paths IIRC are an example)

michalmarcinkowski · 2016-05-20T10:17:18Z

app/config/config.yml

-    - { resource: @SyliusCoreBundle/Resources/config/app/main.yml }
-    - { resource: @SyliusAdminBundle/Resources/config/app/main.yml }
+    - { resource: "security.yml" }
+


parameters.yml shouldn't be there also?

michalmarcinkowski · 2016-05-20T10:27:35Z

src/Sylius/Bundle/CoreBundle/Resources/config/config.yml

+    strict_variables: "%kernel.debug%"
+    exception_controller: "FOS\\RestBundle\\Controller\\ExceptionController::showAction"
+
+winzou_state_machine:


IMO this should be extracted to a separate file.

pjedrzejewski · 2016-05-20T11:12:50Z

I think we should separate app configurations from services/parameters. Not sure if current config/app directory is a good idea, but having one big config.yml next to services.xml, form.xml, etc. can be confusing.
State machines should definitely be in separate files.
Why removing base Kernel class?

michalmarcinkowski · 2016-05-20T12:27:15Z

src/Sylius/Bundle/CoreBundle/Resources/config/sylius.yml

-                    form:
-                        default: Sylius\Bundle\CoreBundle\Form\Type\ProductVariantType
+                    model: Sylius\Component\Product\Model\ProductAssociation
+                    interface: Sylius\Component\Product\Model\ProductAssociationInterface


Not a concern of this PR, but this shouldn't live in ProductBundle?

This way we would couple ProductBundle with AssociationBundle.

Sylius\Component\Product\Model\ProductAssociation - it is already coupled with component :)

And probably with the bundle (composer.json).

pamil · 2016-05-23T14:07:15Z

@pjedrzejewski:

Moved back these files to bundles' config/app/*
Extracted state machine configuration to bundles' config/app/state_machine.yml
The kernel class from CoreBundle was moved to /app/, because it also includes WebBundle / AdminBundle etc. and it's definitely not a property of CoreBundle

pamil · 2016-05-23T14:08:59Z

I think it's all for this iteration, if there aren't any more issues, it would be nice to get this PR merged so I can open the another one on Sylius-Standard.

lchrusciel · 2016-05-23T14:13:52Z

src/Sylius/Behat/Resources/config/profiles/default.yml

-                class: TestKernel
-                path: app/TestKernel.php
+                class: TestAppKernel
+                path: app/TestAppKernel.php


Shouldn't path have "?

It's not required in this case.

michalmarcinkowski · 2016-05-24T08:11:15Z

app/migrations/Version20160523143611.php

@@ -17,7 +8,7 @@
 /**
 * Auto-generated Migration: Please modify to your needs!
 */
-class Version20160516131553 extends AbstractMigration
+class Version20160523143611 extends AbstractMigration


Why this migration was not removed?

It's the first one made after v0.18 was tagged

pjedrzejewski · 2016-05-25T19:56:53Z

src/Sylius/Bundle/ShippingBundle/Resources/config/app/state_machine.yml

+# This file is part of the Sylius package.
+# (c) Paweł Jędrzejewski
+
+jms_serializer:


This should not be here.

…ations

… to current coding standards

…est_cached)

… to /app/config/

michalmarcinkowski · 2016-05-27T08:35:39Z

Good job! 👍

[Sylius] Fixes for changes from #5058

nakashu · 2016-06-06T21:04:06Z

@pamil why where all migrations removed ??? That seems a bit drastic for upgrading.

psyray · 2016-06-11T09:14:57Z

@pamil @pjedrzejewski
Agree with @nakashu, remove migrations is a really bad idea for those who want to upgrade from very old sylius version.
And this part was not really discussed here.
I think this part should be quickly reverted.

pjedrzejewski · 2016-06-11T09:25:52Z

@psyray We will publish old migrations in a separate repo probably, because we need to generate new ones and figure out proper upgrade strategy that considers application + sylius migrations together.

psyray · 2016-06-11T10:14:47Z

OK, right, but meanwhile there is no possibility to cleanly migrate old sylius for those who wants.
Maybe you could put a dir named old with old migrations and a README textfile in migrations folder to permit user to manually copy/paste migrations in main folder ?

peteward · 2016-06-11T10:16:56Z

The old migrations didn't work for anyone with a DB that has content because they didn't consider any data - they just did structure. So you would get a same result by just doing a doctrine:migrations:diff and then working through your issues.

psyray · 2016-06-11T10:24:48Z

I'm not agree with you.
Some old migrations have logic and move data from a table to another, you can look at those with code migrations or translations (like Version20160112154949 Version20151203134947 or Version20141203091118).
Those 2 cases are BC breaks, and old sylius version need those ones to properly upgrade, you couldn't simply drop column in one table and create empty one.
More important in code migration case, due to the UNIQUE database parameter of the column, this will generate a NOT UNIQUE SQL error in any case.

Another example is channel, which insert default channel and update the whole channel tables.
Version20150202213852
Also a BC break

More recent, option_value translation
Version20160203224648
Payment_method update
Version20151015181310

I'm agree that a lot of migrations was obsolete and was just a doctrine:migrations:diff but some are not, and drop the entire migration folder was a mistake.

And finally as @pjedrzejewski said, the needs to put a proper upgrade strategy is inevitable and I think it's urgent

So I think the above migrations file should deletion should be reverted (and maybe another ones)

pjedrzejewski added the DX Issues and PRs aimed at improving Developer eXperience. label May 18, 2016

pamil force-pushed the qa-dx/seek-and-destroy-duplicated-applications branch 5 times, most recently from a6b385d to 9838545 Compare May 20, 2016 09:03

michalmarcinkowski reviewed May 20, 2016
View reviewed changes

pamil force-pushed the qa-dx/seek-and-destroy-duplicated-applications branch from 9838545 to ebd931f Compare May 20, 2016 10:21

michalmarcinkowski reviewed May 20, 2016
View reviewed changes

pamil mentioned this pull request May 20, 2016

[Behat] Perform no deep merging on sylius_multi_container.imports #5079

Merged

pamil force-pushed the qa-dx/seek-and-destroy-duplicated-applications branch 4 times, most recently from 4ec82ac to 4127d04 Compare May 23, 2016 14:05

pamil changed the title ~~[WIP] Executable tests on Sylius-Standard & remove duplicated configs~~ Executable tests on Sylius-Standard & remove duplicated configs May 23, 2016

lchrusciel reviewed May 23, 2016
View reviewed changes

pamil force-pushed the qa-dx/seek-and-destroy-duplicated-applications branch 5 times, most recently from 21b5676 to 6989fc1 Compare May 24, 2016 00:03

michalmarcinkowski reviewed May 24, 2016
View reviewed changes

pjedrzejewski reviewed May 25, 2016
View reviewed changes

pamil added 13 commits May 27, 2016 09:54

[Core] Include application security configuration

b124ee8

[Core] Moved migrations from app/migrations to @SyliusCoreBundle/Migr…

82a5f63

…ations

[Core] Rename state-machine.yml to state_machine.yml, clean according…

9e5ed3d

… to current coding standards

[Search] Prepend Doctrine ORM configuration with MATCH AGAINST function

74abfaa

[Core] Extracted default application configuration

5991230

[Core] Extracted environmental application config (dev, prod, test, t…

9e8b7c1

…est_cached)

[Core] Extracted application parameters configuration

2576b1e

[Core] All settings from SyliusCoreBundle/Resources/config/app/ moved…

5e4fa3b

… to /app/config/

[Sylius] Clean up application kernels, front controllers and autoloading

5dace4c

[Sylius] Distribute config files between bundles

8ede0df

[Behat] Add DefaultSuiteSettingsExtension

603b61f

[Sylius] Extract state machine configuration, make config/app dir

cbe843e

[Sylius] Remove migrations

5c1865b

pamil force-pushed the qa-dx/seek-and-destroy-duplicated-applications branch from c7c73bb to 5c1865b Compare May 27, 2016 07:55

michalmarcinkowski merged commit 0abd1b6 into Sylius:master May 27, 2016

pamil deleted the qa-dx/seek-and-destroy-duplicated-applications branch May 27, 2016 10:10

pamil added a commit to pamil/Sylius that referenced this pull request May 27, 2016

[Sylius] Fixes for changes from Sylius#5058

9b988ed

pamil mentioned this pull request May 27, 2016

[Sylius] Fixes for changes from #5058 #5128

Merged

pamil added a commit to pamil/Sylius that referenced this pull request May 27, 2016

[Sylius] Fixes for changes from Sylius#5058

e06000b

pjedrzejewski pushed a commit that referenced this pull request May 28, 2016

Merge pull request #5128 from pamil/configuration-and-other-stuff

673975b

[Sylius] Fixes for changes from #5058

pamil mentioned this pull request May 30, 2016

[Sylius] Extract CoreBundle's Kernel once again #5140

Merged

michalmarcinkowski mentioned this pull request May 31, 2016

[Core] Add missing configuration parameters #5151

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Executable tests on Sylius-Standard & remove duplicated configs #5058

Executable tests on Sylius-Standard & remove duplicated configs #5058

pamil commented May 18, 2016

pjedrzejewski commented May 18, 2016

pamil commented May 18, 2016

pjedrzejewski commented May 18, 2016

michalmarcinkowski May 20, 2016

michalmarcinkowski May 20, 2016

pjedrzejewski commented May 20, 2016

michalmarcinkowski May 20, 2016 •

edited

Loading

pamil May 23, 2016

michalmarcinkowski May 23, 2016 •

edited

Loading

michalmarcinkowski May 23, 2016

pamil commented May 23, 2016

pamil commented May 23, 2016

lchrusciel May 23, 2016 •

edited

Loading

pamil May 23, 2016

michalmarcinkowski May 24, 2016

pamil May 24, 2016

pjedrzejewski May 25, 2016

pamil May 27, 2016

michalmarcinkowski commented May 27, 2016

nakashu commented Jun 6, 2016

psyray commented Jun 11, 2016 •

edited

Loading

pjedrzejewski commented Jun 11, 2016

psyray commented Jun 11, 2016

peteward commented Jun 11, 2016

psyray commented Jun 11, 2016 •

edited

Loading

Executable tests on Sylius-Standard & remove duplicated configs #5058

Executable tests on Sylius-Standard & remove duplicated configs #5058

Conversation

pamil commented May 18, 2016

pjedrzejewski commented May 18, 2016

pamil commented May 18, 2016

pjedrzejewski commented May 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjedrzejewski commented May 20, 2016

michalmarcinkowski May 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalmarcinkowski May 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pamil commented May 23, 2016

pamil commented May 23, 2016

lchrusciel May 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalmarcinkowski commented May 27, 2016

nakashu commented Jun 6, 2016

psyray commented Jun 11, 2016 • edited Loading

pjedrzejewski commented Jun 11, 2016

psyray commented Jun 11, 2016

peteward commented Jun 11, 2016

psyray commented Jun 11, 2016 • edited Loading

michalmarcinkowski May 20, 2016 •

edited

Loading

michalmarcinkowski May 23, 2016 •

edited

Loading

lchrusciel May 23, 2016 •

edited

Loading

psyray commented Jun 11, 2016 •

edited

Loading

psyray commented Jun 11, 2016 •

edited

Loading