Skip to content
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

Merged
merged 7 commits into from
May 23, 2022

Conversation

lruozzi9
Copy link
Member

Fixes #106

Changes proposed in this pull request:

  • Handle metrical and boolean attribute types as variant axle

@webgriffe/wg-devs what do you think about this PR :shipit:?

@lruozzi9 lruozzi9 added bug Something isn't working enhancement New feature or request labels Apr 14, 2022
@lruozzi9 lruozzi9 self-assigned this Apr 14, 2022
@lruozzi9
Copy link
Member Author

@mmenozzi @LucaGallinari gentle reminder 😀.

src/ValueHandler/ProductOptionValueHandler.php Outdated Show resolved Hide resolved
src/ValueHandler/ProductOptionValueHandler.php Outdated Show resolved Hide resolved
src/ValueHandler/ProductOptionValueHandler.php Outdated Show resolved Hide resolved
return preg_replace('/[^a-z0-9\-_]+/i', '', $word);
}, $pieces);

return implode('_', $slugifiedPieces);
Copy link
Member

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

Copy link
Member

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.

Copy link
Member

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 ?

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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)

@lruozzi9 lruozzi9 force-pushed the 106-unable-to-import-metrical-product-option-value branch from 17ccc50 to 2fa0cbd Compare May 23, 2022 08:29
@lruozzi9 lruozzi9 merged commit 33bd5be into 1.15 May 23, 2022
@lruozzi9 lruozzi9 deleted the 106-unable-to-import-metrical-product-option-value branch May 23, 2022 09:38
@lruozzi9 lruozzi9 changed the title Handle metrical and boolean attribute types as variant axle (#106) Handle metrical and boolean attribute types as variant axis (#106) May 23, 2022
lruozzi9 added a commit that referenced this pull request Jun 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants