Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.27%. Comparing base (642326a) to head (66f09b2).
⚠️ Report is 2 commits behind head on main.

@@            Coverage Diff            @@
##               main     #218   +/-   ##
=========================================
  Coverage     98.27%   98.27%           
  Complexity      994      994           
=========================================
  Files            42       42           
  Lines          2961     2961           
=========================================
  Hits           2910     2910           
  Misses           51       51           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Use of global variables in general is something I typically discourage these days. There's not much discussion here, so I'm guessing it happened in another forum.

My question: Is $GLOBALS being added here for backward compatibility with existing code? Or are we saying that $GLOBALS is unconditionally allowed by Moodle Coding standards?

Personally, I would recommend temporarily allowing, but discouraging the use of $GLOBALS because access to these values is already available from the global keyword and which serves as a very clear semantic highlight of external dependencies for a method. Then, over years, usage could be phased out and changes to global state could be less problematic.

Hi @jrchamp,

Yes, we do discourage its use, but it is still in use in a few places and automatically fixing it will break things, so we to allow it.

Perfect! Thank you for confirming.

Worth noting: the sniff says nothing about whether you should use $GLOBALS or not. PHPCS simply reports that $GLOBALS should be lower case - which if course it shouldn't be; and if you followed the advice and changed it, your code would break.

@cameron1729 Excellent point! 🧠 In that sense, this sniff should include all of the special variables from both PHP itself and Moodle core. I can't begin to thank you for highlighting that note. ❤️

As far as discouraging use, I agree that seems that would be a different Sniff. Perhaps something new like moodle.PHP.DeprecatedVariables. Either way, that's probably something that should be its own issue.

No worries @jrchamp - happy to help :)

I think a new sniff makes sense too. Maybe part of the moodle-extra coding style.