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

feat: rename namespaces and provide aliases #1937

Merged
merged 13 commits into from
Oct 20, 2020
Merged

feat: rename namespaces and provide aliases #1937

merged 13 commits into from
Oct 20, 2020

Conversation

bshaffer
Copy link
Contributor

@bshaffer bshaffer commented Sep 14, 2020

Addresses #692:

  • Adds namespaces, removes underscores
  • Adds autoloading for namespaces
  • Flattens directories for PSR-4

Backwards-compatibility concerns

BC is preserved using aliases.php. This loads the (minimal) 20 classes in this library upfront and creates aliases for them.

Another option is to define a custom autoload.php which lazy-loads the Google_ classes as they're requested:

// src/autoload.php
$classMap = [
    'Google_Client' => 'Google\Client',
    // ...
];

spl_autoload_register(function ($class) use ($classMap) {
    if (isset($classMap[$class])) {
        class_alias($classMap[$class], $class);
        return true;
    }
});

The problem with this is the autoload.php function is invoked on every class-exists. Which is preferred? Tell me your thoughts in the comment section and don't forget to smash that "subscribe" button!

Note: this is independent of google/apiclient-services

@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Sep 14, 2020
@bshaffer bshaffer requested a review from a team September 14, 2020 17:45
@@ -0,0 +1,29 @@
<?php

$classMap = [
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm worried about this map falling out of date if we add new classes. Would be nice to test that all the classes in the src folder within the Google\\ namespace have a mapping. Some combination of RecursiveDirectoryIterator and FileReflector maybe.

Also, maybe just my preference, but I'd reverse the keys and values of this array. Makes more sense to me that the real name is the key, and also corresponds better to the argument order of class_alias().

Copy link

Choose a reason for hiding this comment

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

I'm worried about this map falling out of date if we add new classes

Stupid outsider question (apologies if this isn't appropriate here!): why would a new class still be relevant for the old non-namespaced version? Would this still be maintained in parallel … 🤔

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point, I suppose it's something that we'd need to consider. It would seem strange to have some classes without aliases, but maybe that's good, might be a nudge towards upgrading to use the namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I switched the order of aliases.php

I agree with @mfn that there'd be no reason to add an alias for a new class

@jdpedrie
Copy link
Contributor

jdpedrie commented Oct 7, 2020

I think I have a small preference for the aliases.php file over an autoloader, but it may be because it already exists.

@bshaffer
Copy link
Contributor Author

@jdpedrie I think it makes things easier and simpler to autoload aliases.php up front than to mess with an autoloader. The number of classes is small enough that it's unlikely to have a noticeable impact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants