Fix removing all component tags#4602
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes #4428 where the last tag could not be removed from a component by normalizing tag parsing (trimming + filtering empty values) and ensuring the API accepts an explicitly empty tags payload so existing tag relations can be fully cleared.
Changes:
- Added a
parseTags()helper to trim and drop empty tag values before syncing. - Updated API tag handling to treat an explicitly provided empty
tagsvalue as a valid “clear all tags” request. - Applied the same tag normalization behavior in both dashboard and API component create/update flows.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/Http/Controllers/Dashboard/ComponentController.php | Normalizes dashboard tag input before syncing to allow clearing all tags. |
| app/Http/Controllers/Api/ComponentController.php | Normalizes API tag input and updates tags-handling condition to allow explicit empty payloads to clear relations. |
Comments suppressed due to low confidence (2)
app/Http/Controllers/Api/ComponentController.php:112
- No automated test covers the new/changed behavior where providing an explicitly empty
tagspayload should clear all component tag relations. Adding an API test that creates a component with tags, then updates it withtagsas an empty string and asserts the pivot/tag relations are removed would prevent regressions for #4428.
if (!is_null(Binput::get('tags', null))) {
// The component was added successfully, so now let's deal with the tags.
$tags = $this->parseTags(Binput::get('tags'));
// For every tag, do we need to create it?
$componentTags = array_map(function ($taggable) use ($component) {
return Tag::firstOrCreate([
'name' => $taggable,
])->id;
}, $tags);
$component->tags()->sync($componentTags);
}
app/Http/Controllers/Api/ComponentController.php:150
- No automated test covers the update-path behavior where setting
tagsto an explicit empty value should detach all tags. Consider adding aPUT /api/v1/components/{id}test that verifies existing tags are removed whentagsis sent empty, and that whitespace-only/extra-commas inputs are normalized as expected.
if (!is_null(Binput::get('tags', null))) {
$tags = $this->parseTags(Binput::get('tags'));
// For every tag, do we need to create it?
$componentTags = array_map(function ($taggable) use ($component) {
return Tag::firstOrCreate(['name' => $taggable])->id;
}, $tags);
$component->tags()->sync($componentTags);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @param string $tags | ||
| * | ||
| * @return array | ||
| */ | ||
| protected function parseTags($tags) | ||
| { | ||
| return array_values(array_filter(array_map('trim', preg_split('/ ?, ?/', (string) $tags)), function ($tag) { |
There was a problem hiding this comment.
parseTags() casts $tags to string; if a request submits tags as an array (e.g. tags[]=a&tags[]=b or JSON array), PHP will cast it to the literal string "Array" and this code will create/sync a tag named "Array". Consider explicitly handling arrays (e.g. accept an array of tags) or rejecting non-scalar input with a 400 to avoid silently corrupting tag data.
| * @param string $tags | |
| * | |
| * @return array | |
| */ | |
| protected function parseTags($tags) | |
| { | |
| return array_values(array_filter(array_map('trim', preg_split('/ ?, ?/', (string) $tags)), function ($tag) { | |
| * @param string|array $tags | |
| * | |
| * @return array | |
| */ | |
| protected function parseTags($tags) | |
| { | |
| if (is_array($tags)) { | |
| $flattened = []; | |
| foreach ($tags as $tag) { | |
| if (!is_scalar($tag) && $tag !== null) { | |
| throw new BadRequestHttpException('Invalid tags format.'); | |
| } | |
| $flattened[] = (string) $tag; | |
| } | |
| $tags = implode(',', $flattened); | |
| } elseif (is_scalar($tags) || $tags === null) { | |
| $tags = (string) $tags; | |
| } else { | |
| throw new BadRequestHttpException('Invalid tags format.'); | |
| } | |
| return array_values(array_filter(array_map('trim', preg_split('/ ?, ?/', $tags)), function ($tag) { |
| * @param string $tags | ||
| * | ||
| * @return array | ||
| */ | ||
| protected function parseTags($tags) | ||
| { |
There was a problem hiding this comment.
parseTags() casts $tags to string; if the dashboard form ever submits tags as a non-string (e.g. array due to a UI widget change), PHP will cast arrays to the literal string "Array" and could create an unintended tag named "Array". Consider guarding against non-scalar input (e.g. treat arrays as a list of tags or return an empty list / throw validation error).
| * @param string $tags | |
| * | |
| * @return array | |
| */ | |
| protected function parseTags($tags) | |
| { | |
| * @param mixed $tags | |
| * | |
| * @return array | |
| */ | |
| protected function parseTags($tags) | |
| { | |
| // If an array is provided (e.g. from a tags UI component), treat it as a list of tags. | |
| if (is_array($tags)) { | |
| $tagList = []; | |
| foreach ($tags as $tag) { | |
| // Only accept scalar or null-like values; ignore complex structures. | |
| if (!is_scalar($tag) && $tag !== null) { | |
| continue; | |
| } | |
| $value = trim((string) $tag); | |
| if ($value !== '') { | |
| $tagList[] = $value; | |
| } | |
| } | |
| return array_values($tagList); | |
| } | |
| // For non-array, non-scalar inputs, return an empty list instead of creating tags like "Array". | |
| if (!is_scalar($tags) && $tags !== null) { | |
| return []; | |
| } | |
| // Preserve existing behavior for scalar inputs by casting to string and splitting on commas. |
Issue link id: #4428
Description:
This resolves the inability to remove the final tag from a component.
Tag parsing now normalizes and filters empty values before sync, and updates accept an explicitly empty tags payload so sync can clear all tag relations.
Applied in both API and dashboard component tag update paths for consistent behavior.