Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Two SVG files in core/misc/icons have CRLF line endings. These cause problems when you commit Drupal to a Git repo, because Git generally wants to convert CRLF to LF, leading to a lot of warnings like:
warning: CRLF will be replaced by LF
This is also inconsistent with the other SVG files in Drupal core, and with Drupal's policy as a whole of using LF vs CRLF.
| Comment | File | Size | Author |
|---|---|---|---|
| #23 | 2831598-23.patch | 3.77 KB | alexpott |
| #11 | 2831598-11.patch | 3.77 KB | idebr |
| #11 | interdiff-2-11.txt | 881 bytes | idebr |
Comments
Comment #2
Dane Powell CreditAttribution: Dane Powell at Acquia commentedComment #3
Dane Powell CreditAttribution: Dane Powell at Acquia commentedIf anyone can suggest a way to automatically test for CRLF line endings in future commits, I think that would be smart. Not sure how myself.
Comment #4
Chi CreditAttribution: Chi commentedI wonder if SVG should be configured in the .gitattrbutes file the same way as XML.
You may find this useful https://www.drupal.org/node/1542048.
Comment #6
LoMo CreditAttribution: LoMo as a volunteer commentedYour fix looks good. The patch applies and fixes the only two instances in the project of CRLF line endings.
I don't think this needs to wait for 8.4 release, since this fix is totally non-disruptive.
To verify that these were the only two instances of a CRLF, I used the following terminal command:
find . -not -type d -exec file "{}" ";" | grep CRLFWhat was displayed:
(The third item found was your patch, of course. And after applying your patch, that was the only output from re-running the find/grep command.)
Full disclosure: I'm not a total terminal geek. ;-) I tried the most up-rated answer found here: http://stackoverflow.com/questions/73833/how-do-you-search-for-files-con...
Seems like the main repo needs to be set up to follow the published Git configuration instructions for CRLF? (Or maybe there is something wrong with that configuration and it needs a tweak?)
Comment #7
alexpottThe suggestion in #4 seems very sensible - it will prevent this from every happening again.
Comment #8
LoMo CreditAttribution: LoMo as a volunteer commented@alexpott Given that the scope of this issue was to fix the files in Drupal core which had slipped through whatever system might have been in place to prevent this sort of thing, shouldn't this issue still be considered RTBC? I think we can open another issue (referencing this one) to fix the d.o. Git system to reject any submitted patch which has CLRF line endings. But that would be an issue that only very few (drumm, maybe you, and other core committers, perhaps) would be able to take care of, if I'm not mistaken. Setting this one to "Needs work" sends a message to people who worked on it that there is something to be done (potentially by them). And I don't think that there is, right?
Since this is an issue related to existing files in Drupal core and the "bigger picture" is an issue for drupal.org's repo system, I've gone ahead and created that issue here: #2875025: Add code sniffs for line endings / SVG (on Project: Drupal.org webmasters / Component: Project/Git problem), and I've set this issue's status back to RTBC. (Please let me know if that approach doesn't make sense to you... ;-) )
Comment #9
alexpott@LoMo part of fixing a bug it also to work out if we can prevent causing it again. That can mean adding automated tests or reconfiguring something. In this instance it is configuring something that is missing.
Comment #10
LoMo CreditAttribution: LoMo as a volunteer commented@alexpott I understand your point. And shouldn't the other issue tackle the "prevention issue" (and take further diagnosis from those with the power to do anything about it). I think this issue could just be resolved as something now patched in core, without any need for others to run into issues with those files. As we see from the issue description, this issue was created because those files were causing problems.
The "root issue" is, afaik, something that can only be managed by a few d.o. infrastructure admins (I'm thinking maybe I should have created it in https://www.drupal.org/project/issues/infrastructure, but not sure, since I found the "Project/Git problem" component in the d.o. Webmasters project). But creating that as a separate task ensures that it gets handled. Since it seems commits like that are not happening very often (or there would be a lot more than two files that have this issue—both of these files were in one patch committed quite some time back (25e7311c (webchick 2016-08-31 12:16:46), btw, so I really think this is an "edge case" which may only affect .svg and/or related files), I think it's safe to get this one closed and leave #2875025: Add code sniffs for line endings / SVG open for whenever d.o. admins have time to deal with sussing out a solution for future prevention.
In short: Since this issue is causing current headaches for people deploying D8-based systems, on a regular basis and has been open for 5 months (of the 8 months those files have been in core), and the "root issue" is something that seems to only have happened once in thousands of commits to D8 core, I think THIS issue is somewhat more critical and time-sensitive and the "root cause" issue can wait (probably with no ill ramifications). IMHO, setting this issue to "Needs work" doesn't do anything useful, produces distraction for people attempting to contribute (who can do nothing about the needed work) and fails to address the more immediate needs of the community (that core has some problematic files). I'm not going to set this back to RTBC since my intention is not to annoy you (and I think that might ;-) ), but I'm wondering if you might have missed the part about "These cause problems when you commit Drupal to a Git repo..." in the issue description?
Comment #11
idebr CreditAttribution: idebr at ezCompany commentedAttached patch implements the following changes compared to #2, see interdiff-2-11.txt:
@LoMo I understand your wish to further the Drupal project, but let's leave the executive calls to the core committers. If they agree a bug has not been fully resolved, let's work together to finish up the patch.
Comment #12
LoMo CreditAttribution: LoMo as a volunteer commentedYeah... That should do it. :-) Thank you, @idebr.
@idebr And you're right. In this case, I really thought that the d.o. central repo needed tweaks that couldn't be made from "our end" and that that might mean this issue just stayed open till it got attention for those fixes. My mistake. (The page linked by @chi was related to the .gitconfig file, so I was confused by his reference to the .gitattributes file, which I didn't actually realize was part of the repo we can fix—in this case, I really thought we would be waiting for a d.o sysadmin to make some changes outside of the project.) In my defense, I haven't been doing much with Drupal 8 in some time, so I'm back on the "learning curve" when it comes to understanding the project architecture.
When I've had time, over the years, I've tried to help review patches that had been stuck open for a long time (like this one) and sometimes it's frustrating to me when I spend time on such issues, verify that a patch works and achieves the stated goal, mark them RTBC, and then see them kept open, often for additional years, due to "scope creep" and endless discussion that carries on till no patch still even applies and everyone who originally put so much effort into it has completely lost interest (or no longer had time, as has usually been my case) in dealing with the task. There are so many such issues and it's a source of frustration for me, perhaps especially since reviewing patches is a somewhat "selfless" task in that reviewers often need to do a fair bit to understand and replicate the issue, verify the fix, etc (and only very rarely get any credit for that work). I don't do it for the "credit" but I like to see issues get resolved and see related tickets get created rather than seeing patches bloat to cover the whole project, start getting in conflict with other patches, and require endless re-rolls. I guess my experiences with that kind of thing made me think that this was just another case of "scope creep" blocking an issue. But you are right. In this case, I just misunderstood the remaining task and didn't realize how simple it could be. ;-) Again, my mistake, and thanks for sorting out that last bit that, I hope, can allow this to be closed.
Comment #13
Gábor HojtsyAre all the added code rules apply to svg? (I genuinely cannot tell).
Comment #14
Gábor HojtsyComment #15
alexpott@Gábor Hojtsy yep that's what the
*.svgat the start of the line is indicating.Comment #16
Gábor HojtsyWell, what I meant is do we want 2 space indents in svg, etc? I guess we do then.
Comment #17
alexpott@Gábor Hojtsy ah good point. I think it is valid to have the same format as xml for example. They are very similar. It uses the same DOM where possible as HTML... and
https://en.wikipedia.org/wiki/Scalable_Vector_Graphics
Comment #18
LoMo CreditAttribution: LoMo as a volunteer commentedRe #16 Probably not. That would certainly be inconsistent with any other .svg file committed and, while I don't think it would cause any issues... I think you are right. .svg should probably have a slightly different setting than other .xml files.
Comment #19
Gábor HojtsyAlso whatever code style configuration we have, we need to ensure that the existing SVGs match that, not just in terms of line ending then, which would be important to check before / in case we commit rules that contradict existing files :)
Comment #20
alexpottA single line svg does not contradict the rule as far as I can see. As far as I know all core .svg should be on a single line. But they are not... Here's a line count per file...
Comment #21
alexpottAlso note that .gitattributes does not define coding standards. The tabwidth=2 is an instruction for diffing not for what is in the file. That's a separate issue. So in my mind this issue is still rtbc because the
eol=lfensures that the end of lines will just be line feeds.Comment #22
alexpottSo here's a way to fix and ensure we are compliant.
Comment #23
alexpottA patch created by then doing
git apply ../all.svg.patch --whitespace=fix... results in exactly the same patch as #11.Comment #24
idebr CreditAttribution: idebr at ezCompany commentedRe #16
and #20
There is an effort the decide on svg guidelines / formatting in #2433761: [meta] svg guidelines / requirements. I would like to suggest to create a child issue for #2433761: [meta] svg guidelines / requirements once we decide on coding standards for svg to reformat historical files.
Comment #25
LoMo CreditAttribution: LoMo as a volunteer commentedAnyway, #23 is the same as before, so I guess we are safe with these settings as far as fixing the CLRF issue. And it looks like the SVG guidelines should help ensure that such files are consistently formatted in future. Getting this patch in will make sure that CLRF line endings aren't committed for any of them, either, so let's just get this committed. ;-)
Comment #28
catchCommitted/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks! Added reviewer credit for LoMo.