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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Oct 22, 2018
Merged

Major Craft 3 API Updates #4

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4f7b06e
Updated the maxPowerCaptain method call to the latest Craft 3 API
Oct 20, 2018
3bcaef8
We should only be passing userdata if it exists
Oct 20, 2018
f78ecf2
Rewrote the task infrastructure to use the new Craft 3 queue and jobs
Oct 20, 2018
15c6b32
Fixed two instances where we still had the word “task” instead of “job”
Oct 22, 2018
981886e
If the job fails we should be returning false
Oct 22, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/console/controllers/WalkController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use craft\elements\MatrixBlock;
use craft\elements\Tag;
use craft\elements\User;
use craft\helpers\App;
use topshelfcraft\walk\helpers\WalkHelper;
use topshelfcraft\walk\Walk;
use yii\console\Controller;
Expand Down Expand Up @@ -233,7 +234,7 @@ public function actionWalkElements($elementType = '', $callable = null)
}
else
{
Craft::$app->getConfig()->maxPowerCaptain();
App::maxPowerCaptain();
Walk::notice("Calling [{$callable}] on each {$elementType}.");
if (WalkHelper::craftyArrayWalk($elements, $callable)) return 0;
}
Expand Down
14 changes: 11 additions & 3 deletions src/helpers/WalkHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use craft\tasks\MissingTask;
use topshelfcraft\walk\tasks\CallOnElementTask;
use topshelfcraft\walk\tasks\CallOnValueTask;
use craft\helpers\App;

/**
* WalkHelper
Expand Down Expand Up @@ -53,7 +54,14 @@ public static function craftyArrayWalk(&$array, $callable, $userdata = null)

if (is_callable($callable))
{
return array_walk($array, $callable, $userdata);
Copy link
Member

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...?

Copy link
Author

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 🤔

Copy link
Member

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().

if ($userdata)
{
return array_walk($array, $callable, $userdata);
}
else
{
return array_walk($array, $callable);
}
}
else
{
Expand All @@ -75,9 +83,9 @@ public static function spawnTasks($type, $elements, $settings = [], $valParam =
{

if (!is_array($elements)) $elements = [$elements];

// This could take a while. We'd prefer not to get hung up in the middle...
Craft::$app->getConfig()->maxPowerCaptain();
App::maxPowerCaptain();

foreach($elements as $el)
{
Expand Down