-
Notifications
You must be signed in to change notification settings - Fork 2
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
Major Craft 3 API Updates #4
Conversation
Moved the `tasks/*` to the new Craft 3 `queue/jobs/*` directory and updated the job methods to the new API. Updated the method we use to push new jobs to the queue to the new Craft 3 API. Changed any use of the word `task\tasks` in the repo to `job\jobs` to stay consistent.
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.
(Duplicate comment.)
@@ -54,7 +54,14 @@ public static function craftyArrayWalk(&$array, $callable, $userdata = null) | |||
|
|||
if (is_callable($callable)) | |||
{ | |||
return array_walk($array, $callable, $userdata); |
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.
Did an error come up in your testing? The $userdata
param should be null
(default value from the method signature) if no argument is provided, which should be fine to pass into the array_walk
...?
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.
I did yes; check out the latter half of #2. It's complaining that the parameter passed to the $callable
needs to be a boolean but we're passing null. Based on the array_walk
docs, the $userdata
parameter is optional hence the solution I used here. If there is a better way to solve I'd love to know because it doesn't seem like the cleanest method 🤔
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.
A ha! So, you're right — we need to only pass that parameter downstream if the argument is actually provided — but it has nothing to do with array_walk
.
Specifically, array_walk
doesn't care what kind of argument is passed there. It's the downstream method that cares. (In this case, saveElement
is looking for a boolean param, probably to toggle some validation logic. The fact that we were passing a default null
was fine with array_walk
, but saveElement
complained.)
We can't predict whether a target method will ever expect a second param, so we shouldn't presume to put a default value there. We should let our user do that, in cases where they know it's relevant to the target method.
However, a simple truthiness check isn't a good idea here. (What if the $userdata
is false
, or null
, or []
, or 0
, or anything else that casts to a false boolean? That argument will never be passed through.) The correct check to use here is isset()
.
maxPowerCaptain()
method call to the latest Craft 3 API.$userdata
to the$callable
if it exists, otherwise we can end up with errors.tasks/*
to the new Craft 3queue/jobs/*
directory and updated the job methods to the new API.task\tasks
in the repo tojob\jobs
to stay consistent.Fixes #2