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
Merged

[8.x] Add new VendorTagPublished event #36458

merged 9 commits into from
Mar 4, 2021

Conversation

ryangjchandler
Copy link
Contributor

This pull request introduces a new VendorTagPublished event that is dispatched after successfully running the vendor:publish command.

The event receives the tag that was published, as well as the array of paths that were published (source => location).

The main reasoning behind this new event is allowing third-party packages to run particular actions after some of their publishable files have been actually been published.

E.g. you might set a file to be publishable using the tag package-file and wish to do something with that file after is has been published. This would allow you listen for the VendorTagPublished event from your package and run whatever logic you need.

Question for reviewer: I'm firing the event regardless of whether any files were actually published. Is this okay, or should there be an else that wraps the event dispatch? Left it as is because if you run vendor:publish with an invalid tag, it shows the error "Unable to locate publishable resources." but also has the success message "Publishing complete.".

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

@taylorotwell taylorotwell merged commit 6cbe0eb into laravel:8.x Mar 4, 2021
@ryangjchandler ryangjchandler deleted the feature/vendor-tag-published-event branch March 4, 2021 14:06
@ryangjchandler
Copy link
Contributor Author

Thanks @taylorotwell! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants