[Variation] Introduce code for option values and replace presentation with name #4237
Conversation
pjedrzejewski
added
Enhancement
vikey89
changed the title
|
@pjedrzejewski I think I've finished on this PR. |
|
@pjedrzejewski @michalmarcinkowski I would like to help with something else :) |
| $this->abortIf($this->connection->getDatabasePlatform()->getName() != 'mysql', 'Migration can only be executed safely on \'mysql\'.'); | ||
|
|
||
| $this->addSql('ALTER TABLE sylius_product_option DROP name'); | ||
| $this->addSql('DROP INDEX fulltext_search_idx ON sylius_search_index'); |
There was a problem hiding this comment.
This and line below seem incorrect.
|
@vikey89 Great, thank you so much for your time! Could you please squash the commits and have a look at the migration? Otherwise looks good to go! |
| @@ -105,13 +105,13 @@ public function setValue($value) | |||
| /** | |||
| * {@inheritdoc} | |||
| */ | |||
| public function getName() | |||
| public function getCodeOption() | |||
There was a problem hiding this comment.
This should be getOptionCode I think, wdyt?
|
@pjedrzejewski I squashed the commits and fixes migration and option code. |
| @@ -105,13 +105,13 @@ public function setValue($value) | |||
| /** | |||
| * {@inheritdoc} | |||
| */ | |||
| public function getName() | |||
| public function getOptionCode() | |||
There was a problem hiding this comment.
👍 this is much better name.
|
@pjedrzejewski as a sidenote with optionvalues now being translatable we have to be careful when displaying all option values at once otherwise we risk facing something like this(800+ queries FTW) In light of this my #4158 quickly becomes quite important to merge IMO. |
|
@okwinza is this something that can be handled by setting EAGER loading on the translations and optionValues? It should be then loaded with a single query. |
|
ping @pjedrzejewski |
|
@pjedrzejewski @michalmarcinkowski I would like to collaborate with you more often. |
|
@vikey89 you can try to come up with some decent price slider for SearchBundle for example :D On a serious note it would be nice to have it and #4285 merged already :) |
[Variation] Introduce code for option values and replace presentation with name
|
Thanks Vincenzo! 👍 |
|
I think this PR was merged too fast. What is called migration is not only Create and drop, in some case, like this one, migrations must be Create, select, insert and finally drop like this migration I am rewriting the migration to copy all values from the option_value table into option_value_translation table because in this state, this PR break my project and certainly other project |
|
Issue & PR created |
|
Hey @psyray, sorry for that. Most migrations are autogenerated right now. We want to fix that soon, as we are minimizing the changes to schema right now. All future migrations will migrate the data as well. Thanks for your contribution! |
[Variation] Introduce code for option values and replace presentation with name
[Variation] Introduce code for option values and replace presentation with name
[Variation] Introduce code for option values and replace presentation with name
This PR is based from #4213