[Preview - Part 1] Eliminate globals for $RELASES / $OLDRELEASES / $BRANCHES#1931
[Preview - Part 1] Eliminate globals for $RELASES / $OLDRELEASES / $BRANCHES#1931markrandall wants to merge 1 commit into
Conversation
markrandall
requested review from
derickr,
saundefined,
sgolemon and
sy-records
as code owners
| static $cache = null; | ||
|
|
||
| /* there is no normalisation required here because it's all standard format */ | ||
| return $cache ??= require __DIR__ . '/../../include/version.inc'; |
There was a problem hiding this comment.
I wonder whether this "caching" is needed, considering the return happens straight from an opcache-cached oparray.
There was a problem hiding this comment.
version.inc is the one for the most recent releases - as such it's optimized for updating hashes, and and uses an IIFE each time it is included to format the data into the normalized form.
There was a problem hiding this comment.
No, I specifically meant the static $cache part. I really don't think that's needed (nor wanted).
There was a problem hiding this comment.
Is your position that every time we call the function to get the release data, we should re-include the file, and reinvoke the IIFE that's already in there? Because the static is what prevents that from happening.
| { | ||
| static $cache = null; | ||
| return $cache ??= (function () { | ||
| $results = []; |
There was a problem hiding this comment.
same here, and the use of a closure seems odd to just wrap around a single return value, without it being reused.
There was a problem hiding this comment.
I find the style personally cleaner than a big if block or double return points.
| public static function getBranchOverrides(): array | ||
| { | ||
| static $cache = null; | ||
| return $cache ??= require __DIR__ . '/../../include/branch-overrides.inc'; |
markrandall
force-pushed
the
branch-api-extraction
branch
from
5bc2ef7 to
4660988
Compare
…thout having to rely on global-scoped variables.
markrandall
force-pushed
the
branch-api-extraction
branch
from
4660988 to
a416b4d
Compare
markrandall
changed the title
markrandall
changed the title
| } | ||
|
|
||
| #[Deprecated('Use Branches::getFinalRelease')] | ||
| function get_final_release($branch) { |
There was a problem hiding this comment.
I think this is only used in this file, so perhaps remove it?
| static $cache = null; | ||
|
|
||
| /* there is no normalisation required here because it's all standard format */ | ||
| return $cache ??= require __DIR__ . '/../../include/version.inc'; |
There was a problem hiding this comment.
No, I specifically meant the static $cache part. I really don't think that's needed (nor wanted).
3 participants
Attempts to eliminate the dependence on the $RELEASES, $OLDRELEASES, and $BRANCHES global variables that require injecting in at the top level scope to avoid breaking things.
Changes:
Included: