Closed (fixed)
Project:
Drupal core
Version:
8.0.x-dev
Component:
system.module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
24 May 2012 at 12:00 UTC
Updated:
29 Jul 2014 at 20:43 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
Niklas Fiekas commentedClasses converted.
I have also attached the concatenated output of
git blame -Con the new files, showing that most of the code originated in the old file. Best viewed without automatic linewrapping, without Dreditor or zoomed out, to get an overview.Comment #2
Niklas Fiekas commentedMeh ... should have renamed this to the new class name.
Comment #3
aspilicious commentedI know it's bad to repeat "Mail" in the name of the classes. But the class "Test" looks even worse, it doesn't say what the class does.
options:
1) Find a better name for the class
2) Just name it MailTest, like we did with the tracker module and others. At least this tells us that we are testing mail functionality
Comment #4
Niklas Fiekas commentedHtml to text conversion doesn't rely on mailing => Does not nescessarily belong into the Mail namespace => Now we have two classes, that don't qualify each for an own namespace => Just move them both to common. Also now we don't have Mail/Test or Mail/MailTest, but Common/MailTest instead.
Comment #5
aspilicious commentedGood to go!
Comment #6
Niklas Fiekas commentedThanks.
As sun suggested, here's also a diff with lower thresholds for rename detection.
Comment #7
jhodgdonCommitted to 8.x. Thanks!
Comment #8
jhodgdonUh oh. I just realized that this commit was not done correctly. I used patch -p1 to apply the patch, but it did not do the rename properly. So I had to do a second commit. I backed out the patch above with patch -p1 -R, and then used git apply to re-apply. The net result was:
rename core/modules/system/{tests/mail.test => lib/Drupal/system/Tests/Common/HtmlToTextTest.php}
So, now it's actually fixed. Sorry about that!