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

Migrating carbon and phpdotenv #4

Closed
wants to merge 5 commits into from
Closed

Migrating carbon and phpdotenv #4

wants to merge 5 commits into from

Conversation

justinhartman
Copy link

@justinhartman justinhartman commented Nov 14, 2019

UPDATE: In addition, I have also had to upgrade phpdotenv and upgrading this and carbon were not as simple as this original PR once thought. As such, I have now written a more comprehensive PR which addresses all the changes across the various commits I have made.

Migrating Carbon and Phpdotenv

There have been a number of implications for upgrading carbon and phpdotenv. The changes contained below as well as all the submissions to this PR address the upgrade so all runs smoothly, tests don't fail and upgrading is seamless.

Upgrading Carbon

The change in composer.json–upgrading from ~1.0 to v2.0– is critical as laravel/cashier v10.5.* requires nesbot/carbon ^2.0. Without this upgrade, this package won't work on Laravel 5.8 and upwards.

carbon 1.0 has also been deprecated so this change has no other option but to take place.

Laravel deprecations

The fire method, which was deprecated in Laravel 5.4, of the Illuminate\Events\Dispatcher class in \Http\Controllers\WebhookController.php has been removed. The fix is to use the dispatch method.

This event method now looks like this:

Event::dispatch(
   new Events\Any(
       $event['id'],
       $event['type'],
       $event['live'],
       $event['processed'],
       $event['created'],
       $event['data']
   )
);

Unit test failures

Running this command vendor/bin/phpunit produces a number of errors due to the upgrade of carbon and, more specifically, phpdotenv from version 2 to 3. There have been a number of core changes and deprecations.

phpunit.xml

This error has been resolved:

Warning - The configuration file did not pass validation!
The following problems have been detected:

Line 12:
- Element 'phpunit', attribute 'syntaxCheck': The attribute 'syntaxCheck' is not allowed.

Declaration incompatibility

This issue has been addressed across multiple files where setUp() method was implemented:

PHP Fatal error:  Declaration of Bgultekin\CashierFastspring\Tests\****Test::setUp() must be compatible with Orchestra\Testbench\TestCase::setUp()

The issue below was resolved in changes made to Traits\Database.php:

PHP Fatal error:  Declaration of Bgultekin\CashierFastspring\Tests\Traits\Database::tearDown() must be compatible with Orchestra\Testbench\TestCase::tearDown()

Dotenv constructor changes

The upgrade from version 2 to 3 of phpdotenv meant I had to replace many occurrences of new Dotenv(...) with Dotenv::create(...) since the new native constructor takes a Loader instance.

This meant that $dotenv = new \Dotenv\Dotenv(__DIR__); changed to $dotenv = \Dotenv\Dotenv::create(__DIR__);.

Notably, Issue #306 discusses and implements the Loader::load() and its callers now return an associative array of variables loaded with their values, rather than an array of raw lines from the environment file. This issue hasn't been tested yet in this set of submitted patch changes so I don't know what this outcome will have on the code.

Additional code failures

With all the above changes implemented phpunit now runs successfully but there are 4 of the 70 tests that are failing. Fixing these are outside the scope of the PR.

$ vendor/bin/phpunit
PHPUnit 7.5.17 by Sebastian Bergmann and contributors.

Warning:       opcache.save_comments=0 set; annotations will not work
Error:         No code coverage driver is available

................................................................. 65 / 70 ( 92%)
.....                                                             70 / 70 (100%)

Time: 658 ms, Memory: 18.00 MB

OK (70 tests, 219 assertions)

This change is needed as `laravel/cashier v10.5.*` requires `nesbot/carbon ^2.0`.
`vlucas/phpdotenv` needs to be upgraded to `^3.3` to avoid build errors in this repo and to work with Laravel 5.8.
@justinhartman justinhartman changed the title Update Carbon to v2. Migrating carbon and phpdotenv Nov 15, 2019
@bgultekin
Copy link
Owner

Wow how did I miss that. My apologies. Thanks for the PR.

I will test it locally with different versions of laravel and try to fix the rest of phpunit tests you mentioned at first opportunity.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants