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

Windows line endings cause a problem #70

Open
docwilmot opened this issue Dec 22, 2021 · 3 comments
Open

Windows line endings cause a problem #70

docwilmot opened this issue Dec 22, 2021 · 3 comments

Comments

@docwilmot
Copy link
Member

If you clone a repo from Drupal it comes with Windows line endings. But when you run through CU its parser/writer routines use Linux line endings, so your file ends up with a mix. See this file here:
Capture
Notice all the line endings are LF until the end of the Implements hook_field_extra_fields(). comment, when it switches to CR. This happens for all the Implements hook_wwwww(). lines. It may be this is the only write routine thats messed up since I haven't seen any other problems like this. If you convert the file to Linux line endings first, this doesnt happen.

@klonos
Copy link
Member

klonos commented Feb 11, 2023

@docwilmot I cannot reproduce this on Mac OS 🤔 ...are you running things on Windows?

I just tried to convert https://git.drupalcode.org/project/menu_trail_by_path/-/tree/7.x-3.x, and I got no weird (). lines:

 /**
- * Implements hook_menu()
+ * Implements hook_menu().
  */
 function menu_trail_by_path_menu() {

Can you please let me know of a D7 module that you were able to successfully reproduce this issue with?

@klonos
Copy link
Member

klonos commented Feb 11, 2023

...in the meantime, I have taken a look at the regex used in coder_upgrade_upgrade_hook_alter():

'@\*\s+[iI]mplement.*?(hook_.*?)(\(\)|)(\.|)$@m'`

I have the following remarks for moment (untested, but pretty sure about these):

  • There's no need to be doing [iI] if we add the i regex flag at the end - it will try to match case-insensitive.
  • (\.|) close to the end seems to be trying to match a dot or nothing, which can be achieved with \.? (or (\.)? if you want to be safe). That's some odd way to try to match 1 or 0 instances of something - the most common thing to be doing is to use a ? for that, unless there's something specific that we're trying to achieve here that I'm not aware of.
  • Similarly to the previous remark, (\(\)|) also tries to match () or nothing, which can be achieved with (\(\))?
  • Not sure what (hook_.*?) is trying to do, but I believe that the intention was to have (hook_)?.*, which means "try to match hook_ but optionally, then followed by any number of characters"
  • The m flag at the end might be (part of) the culprit here. See https://javascript.info/regexp-multiline-mode for what it does:

    The multiline mode is enabled by the flag m. It only affects the behavior of ^ and $. In the multiline mode they match not only at the beginning and the end of the string, but also at start/end of line.

Anyway, I will need to be able to reproduce this issue first before I can try things and confirm 100% all of the above. I will also need to have a better understanding of the sequence of things coder_upgrade is trying to do, because as you said @docwilmot:

If you convert the file to Linux line endings first, this doesnt happen.

That ^^ may be the answer/solution.

@docwilmot
Copy link
Member Author

This was of course all "Greek" to me. 😄

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

No branches or pull requests

2 participants