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

FormHelper::radio() return broken id attribute when multibyte value options #6635

Merged
merged 2 commits into from May 27, 2015

Conversation

nojimage
Copy link
Contributor

In CakePHP 2.6.4

echo $this->Form->radio('Model.multibyte', ['男性' => '男性']);

I expects return like,

<input type="hidden" name="data[Model][multibyte]" id="ModelMultibyte_" value=""/>
<input type="radio" name="data[Model][multibyte]" id="ModelMultibyte男性" value="男性" />
<label for="ModelMultibyte男性">男性</label>

But, actual result is,

<input type="hidden" name="data[Model][multibyte]" id="ModelMultibyte_" value=""/>
<input type="radio" name="data[Model][multibyte]" id="ModelMultibyteǔ�性" value="男性" />
<label for="ModelMultibyteǔ�性">男性</label>

I traced the FormHelper::radio() method, and I found the method call Inflector::humanize() with multibyte string.
in Inflector::humanize() using ucwords, but ucwords not support multibyte string.

To solve this problem, Inflector::humanize() will use mb_convert_case() if the function exists.

@ADmad ADmad added this to the 2.6.6 milestone May 25, 2015
@ADmad ADmad added the defect label May 25, 2015
@@ -495,7 +495,12 @@ public static function underscore($camelCasedWord) {
*/
public static function humanize($lowerCaseAndUnderscoredWord) {
if (!($result = self::_cache(__FUNCTION__, $lowerCaseAndUnderscoredWord))) {
$result = ucwords(str_replace('_', ' ', $lowerCaseAndUnderscoredWord));
$result = str_replace('_', ' ', $lowerCaseAndUnderscoredWord);
if (function_exists('mb_convert_case') && Multibyte::checkMultibyte($result)) {
Copy link
Member

Choose a reason for hiding this comment

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

I would just always use mb_convert_case if it exists. There isn't much point in checking the string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried check exists mb_convert_case only, but many test case broken.

mb_convert_case results not equal ucwords.
eg.
mb_convert_case('contact addForm', MB_CASE_TITLE) = 'Contact Addform'
ucwords('contact addForm'); = 'Contact AddForm'

I seems to be better, if a string that contains a multibyte, it does not convert to title case.

Copy link
Member

Choose a reason for hiding this comment

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

Given that the inputs to humanize() are documented to be lower_case_and_underscore we should probably look at updating the tests.

I don't like having the code behave differently if the function & data are different because it makes the result of the the method more difficult to predict. Having the method be as consistent as possible is more important to me, as it makes the behavior clearer and more transparent.

@nojimage
Copy link
Contributor Author

I tried to Inflector methods support multibtye string.
but, some test case failed. ;(

@markstory markstory self-assigned this May 26, 2015
@markstory markstory merged commit c6e4208 into cakephp:2.6 May 27, 2015
markstory added a commit that referenced this pull request May 27, 2015
Use the mbstring shims we already provide to make Inflector more robust
than it currently is. This solves the invalid ID attribute generation in
a way that never varies between environments.

Refs #6635
@markstory
Copy link
Member

I switched the code around a bit to use the existing mb* shims and not cause failing tests. Thanks for the help on this @nojimage 👍

markstory added a commit that referenced this pull request May 27, 2015
Bring the fixes for humanize()/underscore() into 3.0.
ADmad added a commit that referenced this pull request May 27, 2015
Port the Inflector fixes from #6635 to 3.0
@nojimage
Copy link
Contributor Author

thanks!

jadb added a commit that referenced this pull request May 28, 2015
* origin/3.1: (305 commits)
  Update version number to 3.0.6
  New mock objects version causes our test suites to fail.
  Update FloatType.php
  Update DateTimeType.php
  Updated docBlock
  Fix empty query expressions for generating invalid SQL.
  Add tests for empty expression objects in association conditions.
  Add option to disable local XML file parsing.
  Port the Inflector fixes from #6635 to 3.0
  Fix typo in error template file name.
  Add test for ProgressHelper output with options.
  Add missing doc blcoks.
  Fix getOriginal() not preserving nulls.
  Fix incorrect doc blocks and PHPCS.
  Fixed @SInCE in QueryExpressionTest
  Added __call() on QueryExpression to allow for and() and or() to be called transparently. Implements #6477
  Added default message for InvalidCsrfTokenException Updated thrown exception messages to be more descriptive of the cause
  replace DS with DIRECTORY_SEPARATOR in filesystem sub-package
  Added InvalidCsrfTokenException. Implements #6546
  Use ProgressHelper in i18n task.
  ...
@nojimage nojimage deleted the issue-6635 branch May 29, 2015 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants