Repository navigation
Fix vendor:publish dumping all assets for unknown tags - #396
Conversation
When --tag did not match a launcher's publish groups, pathsToPublish fell through to every registered path, so other packages' configs were published alongside the requested tag.
techmahedy
left a comment
There was a problem hiding this comment.
Hi @xentixar, thanks for tracking this down. The --tag fix works, but the --launcher regression below needs fixing before merge.
Problem: vendor:publish --launcher=... now publishes nothing
php pool vendor:publish --launcher="Doppar\Queue\QueueLauncher"On this branch that publishes nothing, but it still prints "Published assets for launcher: ...", so the failure is silent. Picking a launcher from the interactive prompt does the same.
Cause
VendorPublishCommand::publishLauncher() calls $launcher->pathsToPublish($launcherClassName).
$this->publishes is keyed by source path (see ServiceLauncher::publishes()), not by launcher class name, so isset($this->publishes[$provider]) is false for a class name. That call used to rely on falling through to "return everything this launcher registered". The new branch turns that into an empty result:
} elseif ($group || $provider) {
return [];
}Behavior before and after
Checked with a launcher that has one ungrouped and one grouped path:
| Call | Before | After this PR |
|---|---|---|
pathsToPublish(Launcher::class) |
all of the launcher's paths | [] ❌ |
| unknown tag | everything (the bug) | [] ✅ |
| known tag | that group | that group |
| no args | everything | everything |
The new tests don't catch this. They only call pathsToPublish() with an unknown tag, or with a made-up provider plus an unknown tag. None passes a launcher class name on its own.
Suggested fix (either one)
- Drop the new
elseif ($group || $provider)branch. Make the group-only case return[]when the group is missing, and leave provider-only as it was. - Keep your
ServiceLauncherchange. InpublishLauncher(), call$launcherClass->pathsToPublish()with no arguments, since it already holds the launcher instance.
Tests to add
pathsToPublish(SomeLauncher::class)returns the launcher's full set, grouped and ungrouped.- Command-level tests for
VendorPublishCommand:--tag=<unknown>errors and writes nothing.--tag=<known>publishes only that group.--launcher=<class>publishes everything that launcher registered.
Note on --tag=ripple
php pool vendor:publish --tag=ripple
ERROR Unable to locate tag: ripple
That error is probably correct. As far as I can see, Ripple registers its tags as ripple-config and ripple-assets, not ripple. Please try --tag=ripple-config to confirm it publishes only ripple.php, and add that as a test.
Thanks again. The tag fix is the right behavior.
…r publishing per-file output
|
Hi @xentixar, thanks for the fix. Publishing one tag no longer drags in other packages' files, which is the behavior we want. While testing the PR I found a regression, so I pushed a follow-up commit ( What was wrong
What the commit changes
One behavior change to be aware ofUnknown tag and unknown launcher now return exit code 1. Scripts that relied on the old exit 0 would notice. Thanks again for catching the original bug. |
Pull Request Checklist
Description
vendor:publish --tag=...asked every launcher for that tag. When a launcher did not register the tag,pathsForProviderOrGroup()returnednull, andpathsToPublish()fell back to merging all of that launcher's publishable paths.So publishing e.g.
ripple-configalso tried to publish unrelated packages such as Insight'sinsight.php.Return an empty array for unknown tag/launcher lookups so only matching groups are published. Publishing with no tag/launcher still publishes everything.
Checklist