-
Notifications
You must be signed in to change notification settings - Fork 1
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
Various fixes required to get supported modules data to be correctly utilised #51
Various fixes required to get supported modules data to be correctly utilised #51
Conversation
66a4b4a
to
3c9b13e
Compare
@@ -1,13 +1,13 @@ | |||
<?php | |||
|
|||
// run on two consecutive days of the week | |||
$dayOfWeek = predictable_random_int(6); | |||
$dayOfWeek = predictable_random_int('dispatch-ci', 6); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use basename(__FILE__)
instead, saves having the same magic string multiple times. Change all of these if you change any.
Doesn't ultimately matter though so I won't hold up merging if you leave these as they are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already tried that, it doesn't work due to the use of eval()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yikes.... we should probably not be using eval but I guess that's something for a separate card.
I guess we just have to hope we don't forget and add code relying on basename
in another script or during refactoring in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
basname()
isn't the problem, the problem is __FILE__
will be the file with eval()
in it, rather than the script file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, pretend I said __FILE__
then, my remark still stands. :p
Nothing to change in this PR, just something we'll have to watch out for in the future.
330c6c7
to
c95c982
Compare
@@ -1,13 +1,13 @@ | |||
<?php | |||
|
|||
// run on two consecutive days of the week | |||
$dayOfWeek = predictable_random_int(6); | |||
$dayOfWeek = predictable_random_int('dispatch-ci', 6); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yikes.... we should probably not be using eval but I guess that's something for a separate card.
I guess we just have to hope we don't forget and add code relying on basename
in another script or during refactoring in the future.
218211c
to
ef8a7b6
Compare
ef8a7b6
to
8206cb1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Issue silverstripe/.github#242
Remove the randomness from the function that's supposed to be predictable
Contains a second commit with various fixes for running against the new tooling, workflow and misc repos