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.

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

[8.x] Add new VendorTagPublished event #36458

Merged
merged 9 commits into from
Mar 4, 2021
6 changes: 5 additions & 1 deletion src/Illuminate/Foundation/Console/VendorPublishCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use Illuminate\Console\Command;
use Illuminate\Filesystem\Filesystem;
use Illuminate\Foundation\Events\VendorTagPublished;
use Illuminate\Support\Arr;
use Illuminate\Support\ServiceProvider;
use League\Flysystem\Adapter\Local as LocalAdapter;
Expand Down Expand Up @@ -158,13 +159,16 @@ protected function parseChoice($choice)
protected function publishTag($tag)
{
$published = false;
$pathsToPublish = $this->pathsToPublish($tag);

foreach ($this->pathsToPublish($tag) as $from => $to) {
foreach ($pathsToPublish as $from => $to) {
$this->publishItem($from, $to);

$published = true;
}

$this->laravel['events']->dispatch(new VendorTagPublished($tag, $pathsToPublish));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a couple of concerns here: first of all it's unsure if anything is published at this point. As such it's better that this line is part of the else condition of the if statement below.

Secondly, the $pathsToPublish isn't necessarily all of the paths that were published (see the contents of publishItem).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryangjchandler I answered this before reading your main PR comment, sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@driesvints No worries, wanted to get your opinion on it because the console output is actually a bit misleading.

An alternative solution might be only passing the $tag through to the event, technically speaking if you're listening for a particular tag, then you are 99% of the time going to know what might have been published.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@driesvints Okay, I've moved the event into the else arm and also updated the comment on the $paths property, so instead of stating that the array contains the paths that were published, it instead states that the $paths property holds all of the publishable paths registered by that tag.

Is this better?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryangjchandler I'll let Taylor review this one, thanks.


if ($published === false) {
$this->error('Unable to locate publishable resources.');
}
Expand Down
32 changes: 32 additions & 0 deletions src/Illuminate/Foundation/Events/VendorTagPublished.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
<?php

namespace Illuminate\Foundation\Events;

class VendorTagPublished
{
/**
* The vendor tag published.
*
* @var string
*/
public $tag;

/**
* The paths that were published.
*
* @var array
*/
public $paths;

/**
* Create a new event instance.
*
* @param string $tag
* @return void
*/
public function __construct($tag, $paths)
{
$this->tag = $tag;
$this->paths = $paths;
}
}