-
Notifications
You must be signed in to change notification settings - Fork 2
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.
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
Handle metrical and boolean attribute types as variant axis (#106) #132
Handle metrical and boolean attribute types as variant axis (#106) #132
Conversation
@mmenozzi @LucaGallinari gentle reminder 😀. |
because Akeneo does not support multiselect attributes as variant axle
return preg_replace('/[^a-z0-9\-_]+/i', '', $word); | ||
}, $pieces); | ||
|
||
return implode('_', $slugifiedPieces); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Qui c'è un potenziale problemino: si permette il carattere _ all'interno dei singoli frammenti, e lo si usa anche come separatore.
In pratica, se qualcuno passa due $pieces che valgono, ad esempio 'a_b' e 'c', questi verrebbero compattati nel codice 'a_b_c'. Però si otterrebbe lo stesso codice anche con i $pieces 'a' e 'b_c'.
Per evitare questo bisognerebbe usare un separatore che non può comparire anche all'interno dei singoli pieces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid we can't because option codes can only be letters, numbers, dashes and underscores. Anyway I think this is a very unlikely case that we can safely ignore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or... maybe we should simplify it: considering that this issue was raised by the introduction of metric attributes, and that the only characters that we need to exclude are characters that appear in numeric values, that's to say dots and maybe commas, wouldn't it be better to make a simple str_replace
to remove only dots and commas? What do you think @LucaGallinari and @azambon ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also be sure to not introduce a BC break by changing the logic the production option value code is generated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also be sure to not introduce a BC break by changing the logic the production option value code is generated.
Actually only the select attribute type is supported by the ProductOptionValueHandler
and in Akeneooption code may contain only letters, numbers and underscores
. So this change should not cause any BC in the previous code generation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only BC could be on the "slugification" of the option value code for select attribute, cause boolean and metric values were not supported.
For select attributes, before we did:
$fullValueCode = $optionCode . '_' . $partialValueCode;
where $partialValueCode is the code of the value. Given that $optionCode and $partialValueCode are two codes, they could not (and actually cannot) contains dots and points.
Now we do:
$slugifiedPieces = array_map(static function (string $word): string {
return str_replace(['.', ','], '', $word);
}, $pieces);
return implode('_', $slugifiedPieces);
where $pieces "contains" $optionCode and $partialValueCode, and the new str_replace is idempotent for select attributes cause, as said before, the two codes cannot contain dots and points. Thus, the result is the same before and now (this is "softly" asserted in some phpspec tests that checks the generated code for select attribute, here and here)
17ccc50
to
2fa0cbd
Compare
Merge #132 (v1.15.0) in master
Fixes #106
Changes proposed in this pull request:
@webgriffe/wg-devs what do you think about this PR ?