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

Add namespaces #49

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add namespaces #49

wants to merge 2 commits into from

Conversation

Nacoma
Copy link
Contributor

@Nacoma Nacoma commented May 5, 2017

Fixes #45, #46, #47

Added namespaces and updated various places that instantiate classes using strings to use the new namespace.

@mootrichard
Copy link
Contributor

@Nacoma Were you able to install this using Composer? I tried installing into a test Laravel project of mine, and I still get:

 ~/P/L/blog  composer install --prefer-source
Loading composer repositories with package information
Updating dependencies (including require-dev)
Package operations: 1 install, 0 updates, 0 removals
  - Installing shippo/shippo-php (dev-master 7f39262): Cloning 7f3926219e from cache
Writing lock file
Generating optimized autoload files
> Illuminate\Foundation\ComposerScripts::postUpdate
> php artisan optimize


  [Symfony\Component\Debug\Exception\FatalErrorException]
  Class 'Shippo' not found


Script php artisan optimize handling the post-update-cmd event returned with error code 255

It seems there is something wrong with trying to import it, which is something that we want to explore fixing. If you happen to be familiar with that issue, it would be great to address that.

@Nacoma
Copy link
Contributor Author

Nacoma commented May 9, 2017

@mootrichard

You most likely have a service provider registered in your test project that has not been updated to reflect the changed namespace.

Example:

<?php

use Illuminate\Support\ServiceProvider;

class ShippoServiceProvider extends ServiceProvider
{
    public function register()
    {
        \Shippo::setApiKey('key');
    }
}

should be:

<?php

use Illuminate\Support\ServiceProvider;
use Shippo\Shippo;

class ShippoServiceProvider extends ServiceProvider
{
    public function register()
    {
        Shippo::setApiKey('key');
    }
}

Failing to update the application will result in the error you're seeing above and (depending on your Laravel version) below.

▶ composer update
Loading composer repositories with package information
Updating dependencies (including require-dev)                                         
Package operations: 0 installs, 0 updates, 0 removals
Generating autoload files
ocramius/package-versions:  Generating version class...
ocramius/package-versions: ...done generating version class
> php artisan clear-compiled

                                                           
  [Symfony\Component\Debug\Exception\FatalThrowableError]  
  Class 'Shippo' not found                                 
                                                           

Script php artisan clear-compiled handling the post-update-cmd event returned with error code 1
▶ php artisan optimize

                                                           
  [Symfony\Component\Debug\Exception\FatalThrowableError]  
  Class 'Shippo' not found                                 
                                                           

@mootrichard
Copy link
Contributor

Thank you for that, I guess my unfamiliarity with PHP is showing there. I was able to get that up and running fine now.

We are going to have someone from our team review the PR further just to double check everything is good to release. We'll likely need to up the version number with the addition of adding namespacing.

Thanks again for contributing here, we really appreciate it.

@casperbakker
Copy link

May I recommend to change the class names as well? You probably do not want to change the class names ever again since it breaks backwards compatibility.

A class name like Shippo_Transaction was the poor boys version of namespacing to avoid class name collisions. But since we have namespacing, it would be better to turn this into Shippo\Transaction instead of Shippo\Shippo_Transaction. In my opinion anyway.

I think this is a good moment (and maybe the only one for a long time), to think longer about the class names and make them really neat.

Maybe there are some other weird things in the naming that we can fix in this mayor breaking change as well.

@Nacoma
Copy link
Contributor Author

Nacoma commented May 11, 2017

I had similar thoughts. An overall rewrite would help this library a lot. There is a lot of duplicate code between resources and manual bootstrapping to make the API calls. The http implementation is very rigid and offers the end user little flexibility, certainly violating the SOLID principles (see issue #1).

I think something similar to the current java implementation, and making use of available HTTP libraries, would make this much more developer friendly.

@Nacoma
Copy link
Contributor Author

Nacoma commented May 11, 2017

Either way I agree though. Some renaming and additional structuring would be nice, and will have to happen on a major version bump. It's now or ~never.

@casperbakker
Copy link

As long as the public interface of the library has the logical public methods and good naming, the refactoring behind the scenes can come later.

@mootrichard What are your thoughts on this? If renaming is fine with you, maybe @Nacoma and I can work on the PR together to name the classes and methods?

@mootrichard
Copy link
Contributor

I am actually fully onboard with creating a better naming scheme. I find it odd to call Shippo::setApiKey(env('SHIPPO_API_KEY')); after having to import Shippo\Shippo. By all means, feel free to adjust namespacing. I view the upgrade to namespacing to be a breaking change anyways, so it might as well be done better.

Also, I would agree that the refactoring behind the scenes should come later. So long as there isn't a big gap between the previous interface and a newer interface (like the new interface is just more semantic) then it should be fine.

@lukeholder
Copy link

What is the status of package, is it being updated?

@Nacoma
Copy link
Contributor Author

Nacoma commented Jun 13, 2019

@lukeholder It appears that it hasn't been updated in at least a year. We've been using it just fine in production for quite a while now on 7.1, but it does seem to be falling behind with no attempts at bringing it up to modern standards.

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

Successfully merging this pull request may close these issues.

1.3.1 cannot find Shippo_Address
4 participants