fix(dev): ensure Ads and Common Protos new component parameters are derived correctly by cy-yun · Pull Request #9691 · googleapis/google-cloud-php

cy-yun · 2026-09-14T22:39:55Z

Ensures that component parameters for Google Ads APIs and Common Protos libraries are derived correctly when running component:new.

Fixes #9387

Summary of Changes

Google Ads APIs (Google\Ads):
- Sets Composer package vendor to googleads/ (e.g. googleads/ad-manager, googleads/data-manager, googleads/marketingplatform-admin).
- Correctly hyphenates trailing -manager and strips ads- prefix.
- Sets GitHub repository to googleapis/php-ads-[project-name].
- Normalizes package name even if a versioned proto package is provided.
Common Protos (No Protobuf Service):
- Adds hasGapicClient detection to check for protobuf service definitions in proto contents.
- Strips trailing type and common proto and PHP namespace segments.
- Appends CommonProtos to component name (e.g. GeoCommonProtos, ShoppingCommonProtos).
- Appends -common-protos to Composer package and GitHub repository names.
- Allows empty api_shortname without erroring when no default_host is defined in the proto.
- Sets library_type to CORE in .repo-metadata-full.json.
Tests:
- Added unit test cases in NewComponentTest and ComponentNewCommandTest covering Ads APIs and Common Protos libraries.

…erived correctly - Support Google\Ads namespace by using vendor googleads/, hyphenating project name, and setting github repo to googleapis/php-ads-* - Support CommonProtos by detecting missing protobuf service definitions, stripping type/common subpackages, appending CommonProtos to component name, and setting repo metadata library_type to CORE - Allow empty api_shortname when no default_host is defined Fixes #9387

bshaffer

This looks good but the changes are way too complex for what I had in mind. I think there are a lot of opportunities to simplify things

bshaffer · 2026-09-15T18:13:53Z

+        if (str_starts_with($phpNamespace, 'Google\\Ads')) {
+            if (count($parts) > 1 && 'v' === strtolower($parts[count($parts) - 1][0] ?? '')) {
+                array_pop($parts);
+            }
+            $name = str_replace(
+                ['google.', 'devtools.cloud', '.'],
+                ['', 'cloud-', '-'],
+                implode('.', $parts)
+            );
+            if (str_starts_with($name, 'ads-')) {
+                $name = substr($name, 4);
+            }
+            if (str_ends_with($name, 'manager') && !str_ends_with($name, '-manager')) {
+                $name = substr($name, 0, -7) . '-manager';
+            }
+            if (!$hasGapicClient && !str_ends_with($name, '-common-protos')) {
+                $name .= '-common-protos';
+            }
+            return 'googleads/' . $name;
+        }


This is overly complicated. We don't need special handling for all of these cases, but rather should just use googleads instead of google if the phpNamespace starts with Google\Ads

bshaffer · 2026-09-15T18:21:42Z

+        while (count($parts) > 1) {
+            $last = end($parts);
+            if ('v' === $last[0]) {
+                array_pop($parts);
+            } elseif (!$hasGapicClient && count($parts) > 2 && in_array($last, ['type', 'common'])) {
+                array_pop($parts);
+            } else {
+                break;
+            }


This is also over complicated. Could simplify to

Suggested change

while (count($parts) > 1) {

$last = end($parts);

if ('v' === $last[0]) {

array_pop($parts);

} elseif (!$hasGapicClient && count($parts) > 2 && in_array($last, ['type', 'common'])) {

array_pop($parts);

} else {

break;

}

$last = end($parts);

if ('v' === $last[0] || in_array($last, ['type', 'common'])) {

array_pop($parts);

}

bshaffer · 2026-09-15T18:24:07Z

+        while (count($parts) > 1) {
+            $last = end($parts);
+            if ('v' === strtolower($last[0])) {
+                array_pop($parts);
+            } elseif (!$hasGapicClient && count($parts) > 2 && in_array(strtolower($last), ['type', 'common'])) {
+                array_pop($parts);
+            } else {
+                break;
+            }


We do not want to modify the proto namespace, we should take what the PHP namespace is from the file

bshaffer · 2026-09-15T18:26:30Z

+        $hasGapicClient = !str_ends_with($new->componentName, 'CommonProtos');
+        if (!$hasGapicClient && !str_ends_with($new->displayName, 'Common Protos')) {
+            $new->displayName .= ' Common Protos';
+        }
+        $new->composerPackage = self::getComposerPackage(
+            $new->protoPackage,
+            $new->phpNamespace,
+            $hasGapicClient
+        );


This logic is a bit odd, as it relies on the user to pass in CommonProtos as the component name. But if this is done, then we can also expect the user to pass in Common Protos as the display name, so there's no reason for us to write logic to do so.

Done, removed the display name mutation from fromOptions.

bshaffer · 2026-09-15T18:26:46Z

+    private static function getComposerPackage(
+        string $protoPackage,
+        string $phpNamespace,
+        bool $hasGapicClient = true


instead of $hasGapicClient, let's just make it $isCommonProtos

Updated to $isCommonProtos across the class.

…dback

cy-yun · 2026-09-15T21:13:25Z

Updated to simplify per review feedback:

Renamed $hasGapicClient to $isCommonProtos across the class, eliminating inverted conditions.
Moved CommonProtos naming logic into getDisplayName and getComponentName.
Removed display name mutation from fromOptions.
Simplified getComposerPackage and extraction methods (extractPackageNameFromProtoContents, extractPhpNamespaceFromProtoContents) to remove verbose while loops.

bshaffer · 2026-09-17T18:48:39Z

+        $parts = explode('.', $protoPackage);
+        if ($isCommonProtos && in_array(end($parts), ['type', 'common'])) {
+            array_pop($parts);
+        }
+        if (str_starts_with($phpNamespace, 'Google\\Ads')) {
+            if (count($parts) > 1 && 'v' === strtolower(end($parts)[0])) {
+                array_pop($parts);
+            }
+            $name = str_replace(
+                ['google.ads.', 'ads.', 'google.', '.'],
+                ['', '', '', '-'],
+                implode('.', $parts)
+            );
+            if (str_ends_with($name, 'manager') && !str_ends_with($name, '-manager')) {
+                $name = substr($name, 0, -7) . '-manager';
+            }
+            $vendor = 'googleads';
+        } else {
+            $name = str_replace(
+                ['google.', 'devtools.cloud', '.'],
+                ['', 'cloud-', '-'],
+                implode('.', $parts)
+            );
+            $vendor = 'google';
+        }
+
+        if ($isCommonProtos && !str_ends_with($name, '-common-protos')) {
+            $name .= '-common-protos';
+        }
+        return $vendor . '/' . $name;


This logic is way too complex. I think AI is trying to cover ALL our current packages (and that's why there's logic for "ads manager" in there, which is not necessary). Something like this should work fine:

Suggested change

$parts = explode('.', $protoPackage);

if ($isCommonProtos && in_array(end($parts), ['type', 'common'])) {

array_pop($parts);

}

if (str_starts_with($phpNamespace, 'Google\\Ads')) {

if (count($parts) > 1 && 'v' === strtolower(end($parts)[0])) {

array_pop($parts);

}

$name = str_replace(

['google.ads.', 'ads.', 'google.', '.'],

['', '', '', '-'],

implode('.', $parts)

);

if (str_ends_with($name, 'manager') && !str_ends_with($name, '-manager')) {

$name = substr($name, 0, -7) . '-manager';

}

$vendor = 'googleads';

} else {

$name = str_replace(

['google.', 'devtools.cloud', '.'],

['', 'cloud-', '-'],

implode('.', $parts)

);

$vendor = 'google';

}

if ($isCommonProtos && !str_ends_with($name, '-common-protos')) {

$name .= '-common-protos';

}

return $vendor . '/' . $name;

$parts = explode('.', $protoPackage);

$vendor = 'google';

if (str_starts_with($phpNamespace, 'Google\\Ads')) {

$vendor = 'googleads';

}

$name = str_replace(

['google.', 'devtools.cloud', 'google.ads.', '.'],

['', 'cloud-', '', '-'],

implode('.', $parts)

);

if ($isCommonProtos && !str_ends_with($name, '-common-protos')) {

$name .= '-common-protos';

}

return $vendor . '/' . $name;

Updated. Placed google.ads. before google. in str_replace so the longer prefix is matched and stripped first in PHP sequential array replacement, and passed $protoPackage directly.

cy-yun requested a review from a team as a code owner September 14, 2026 22:39

bshaffer reviewed Sep 15, 2026

View reviewed changes

refactor(dev): simplify component parameter derivation per review fee…

e6eddbd

…dback

Merge branch 'main' into dev-derive-ads-common-protos

39f42c6

bshaffer reviewed Sep 17, 2026

View reviewed changes

refactor(dev): simplify composer package derivation per review feedback

2950643

Navigation Menu

fix(dev): ensure Ads and Common Protos new component parameters are derived correctly - #9691

fix(dev): ensure Ads and Common Protos new component parameters are derived correctly#9691
cy-yun wants to merge 4 commits into
mainfrom
dev-derive-ads-common-protos

cy-yun commented Sep 14, 2026

Uh oh!

bshaffer left a comment

Uh oh!

bshaffer Sep 15, 2026

Uh oh!

bshaffer Sep 15, 2026

Uh oh!

bshaffer Sep 15, 2026

Uh oh!

bshaffer Sep 15, 2026

Uh oh!

cy-yun Sep 15, 2026

Uh oh!

bshaffer Sep 15, 2026

Uh oh!

cy-yun Sep 15, 2026

Uh oh!

Uh oh!

Uh oh!

cy-yun commented Sep 15, 2026

Uh oh!

bshaffer Sep 17, 2026

Uh oh!

cy-yun Sep 17, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

cy-yun commented Sep 14, 2026

Summary of Changes

Uh oh!

bshaffer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cy-yun commented Sep 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cy-yun Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cy-yun Sep 17, 2026 •

edited

Loading