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

FIX-472 broken product form for cyrillic taxonomy #475

Merged
merged 1 commit into from
Oct 28, 2013

Conversation

asidorov01
Copy link
Contributor

I replaced name to id, this is much more safe, because name could have non-latin character.

See #472

@@ -56,7 +56,8 @@ public function buildForm(FormBuilderInterface $builder, array $options)
$builder->addModelTransformer(new TaxonSelectionToCollectionTransformer($taxonomies));

foreach ($taxonomies as $taxonomy) {
$builder->add(Urlizer::urlize($taxonomy->getName()), 'choice', array(
/* @var $taxonomy Taxonomy*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to add the comment here, but i understand if it is just for autocomplete

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is for IDE, yes!

@cordoval
Copy link
Contributor

👍 👶

pjedrzejewski pushed a commit that referenced this pull request Oct 28, 2013
FIX-472 broken product form for cyrillic taxonomy
@pjedrzejewski pjedrzejewski merged commit d4c27f3 into Sylius:master Oct 28, 2013
@pjedrzejewski
Copy link
Member

I think there is no better option than that for now, thanks Alex!

@winzou
Copy link
Contributor

winzou commented Oct 28, 2013

A better option wouldn't be to update the Urlizer::urlize method to support cyrillic?

@asidorov01 asidorov01 deleted the FIX-472_cyrillic_taxonomy branch October 28, 2013 09:25
@pjedrzejewski
Copy link
Member

@winzou Or maybe using Doctrine extension slugizer? I think it handles Cyrillic?

@stloyd
Copy link
Contributor

stloyd commented Oct 28, 2013

@pjedrzejewski If you think about Gedmo ones, this PR just replaced them, so I guess that it was not working as expected =)

@winzou My guess is that if pcre is not compiled with utf8 this will block that Urlizer as it depends on it (but it's guessing mostly =)).

@asidorov01
Copy link
Contributor Author

Guys, it should use transliterator, and than use Urlizer.

So my first code was like $taxons[strtolower(Urlizer::transliterate((string)$taxonomy->getName()))] = array();
I think you kill me for that code, so I made this simpler with ids.

@pjedrzejewski
Copy link
Member

Well, if it leaves us with more user-friendly ids, I don't think it's bad. @stloyd @winzou?

@stloyd
Copy link
Contributor

stloyd commented Oct 28, 2013

@asidorov01 Your code looks (almost) acceptable (this in comment), I think that strtolower (but even if it's required it can be set as some variable & reused elsewhere) is not required there, same for (string) forcing as if getName() returns something different, there is problem with that code IMO.

@pjedrzejewski IIUC this PR adds (IIRC this in the html spec) not acceptable characters into id's/names in html markup, so it's not correct fix IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants