The correct naming is PhpTransliteration and not PHPTransliteration.

Remaining tasks

  • Rename all instances of PHPTransliteration to PhpTransliteration.
  • Rename files with PHPTransliteration in the name to PhpTransliteration.
  • Commit.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task, because this is a coding standards change.
Issue priority Not critical because coding standard changes are not critical.
Disruption Not disruptive since the rename will not cause anything to break - class names are not case sensitive in PHP.

The renaming is not an API break and the disruption is minimal, so it is allowed in the beta.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tadityar’s picture

Assigned: Unassigned » tadityar
tadityar’s picture

FileSize
108.34 KB

Hello,
This is the patch for renaming all 'PHPTransliteration' inside drupal files.
There is 2 renaming left which are renaming /core/lib/Drupal/Component/Transliteration/PHPTransliteration.php to PhpTransliteration.php and renaming /core/lib/Drupal/Core/Transliteration/PHPTransliteration.php to PhpTransliteration.php

I can't seem to do it though because when I renamed the file using
git mv -f PHPTransliteration PhpTransliteration
and run the patch, it'll return 'already exists in working directory'
Is there any way I can work around this?

Thank You

tadityar’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: drupal-renaming-2388169.patch, failed testing.

daffie’s picture

Thank you for making the patch. I wanted to ask you for rename the files did you do:

git mv -f PHPTransliteration PhpTransliteration

of did you do

git mv -f PHPTransliteration.php PhpTransliteration.php

And what was your working directory?
In my local environment is it working fine.

The testbot failed because you have now a class PhpTransliteration in a file named PHPTransliteration.php.

tadityar’s picture

Thank you for reviewing my patch too. I did the second one, to be exact since I did it from the drupal directory I did
git mv -f /path/to/Transliteration/PHPTransliteration.php /path/to/Transliteration/PhpTransliteration.php

The patch that I posted is not the one that include the change though because it fails when I was applying the patch.

daffie’s picture

@tadityar: Just rename the file and make the patch. If you do not succeed with "git mv" use another method of renaming the files. The patch file will still work. Instead of a rename it has then a deleted file and a new file. But it still works.

tadityar’s picture

Status: Needs work » Needs review
FileSize
19.61 KB

This is my update of renaming the file.
Let's see if the test will pass.. I used a new method using
git mv PHPTransliteration.php PHPTransliteration.php.tmp && git mv PHPTransliteration.php.tmp PhpTransliteration.php

In case the test fails,
Do you have any suggestion regarding this?

Thank you,

daffie’s picture

Status: Needs review » Needs work

@tadityar: The files are deleted and created, so combined they are renamed. Very good of you. But where are all the other changes? All the renaming of PHPTransliteration to PhpTransliteration is gone!

The last submitted patch, 8: drupal-renaming-2388169.patch, failed testing.

tadityar’s picture

FileSize
122.69 KB

My Bad! That's why I thought why was the file so small?.. I had many different tabs on the same file and the file that I checked was the other file with same name..

tadityar’s picture

Status: Needs work » Needs review
daffie’s picture

Status: Needs review » Reviewed & tested by the community

All occurring instances of PHPTransliteration have been replaced by PhpTransliteration.
Two files have been renamed.
It all looks good to me, so I give it RTBC.

amateescu’s picture

Status: Reviewed & tested by the community » Needs work

@tadityar, can you please generate a patch with git diff -C. That will make it easier to see what are the actual changes to \Drupal\Component\Transliteration\PHPTransliteration and it also doesn't change the entire revision history of that file.

daffie’s picture

Status: Needs work » Reviewed & tested by the community

@amateescu: This is her first patch and it did not go very easy. The patch does what is has to, so give her some slack. :-)

amateescu’s picture

@daffie, I'm sorry but committing the current patch means that we will lose all the version history of that file.

amateescu’s picture

Also, I'll be happy to assist @tadityar if any help is needed for re-rolling this patch.

I would recommend reading the instructions from https://www.drupal.org/documentation/git/configure and apply at least the "Optimize diffs for renamed and copied files" part there to your git configuration. That will have the same effect as the command I proposed above (git diff -C) when you create a new patch.

tadityar’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
108.79 KB

@amateescu I've enable all the configurations and this is my revised patch.
Sorry I didn't know about that.

Status: Needs review » Needs work

The last submitted patch, 18: drupal-renaming-2388169.patch, failed testing.

tadityar’s picture

FileSize
108.79 KB
tadityar’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 20: drupal-renaming-2388169.patch, failed testing.

tadityar’s picture

Status: Needs work » Needs review
FileSize
108.67 KB

reverted configuration and used diff --staged -C instead

slashrsm’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

amateescu’s picture

Nice! Great work, @tadityar :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 23: drupal-renaming-2388169.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 23: drupal-renaming-2388169.patch, failed testing.

tadityar’s picture

Status: Needs work » Needs review
FileSize
108.67 KB

Re-rolling the patch.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Reroll looks good :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 29: drupal-renaming-2388169-29.patch, failed testing.

Status: Needs work » Needs review

The last submitted patch, 18: drupal-renaming-2388169.patch, failed testing.

The last submitted patch, 20: drupal-renaming-2388169.patch, failed testing.

daffie’s picture

Status: Needs review » Reviewed & tested by the community

Was before failed retest RTBC. After retest is it all green again, So back to RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 29: drupal-renaming-2388169-29.patch, failed testing.

Status: Needs work » Needs review
daffie’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

tadityar’s picture

Assigned: tadityar » Unassigned
alexpott’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Committed 37c5acf and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 37c5acf on 8.0.x
    Issue #2388169 by tadityar: Rename PHPTransliteration to...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.