Updated to run with Laravel 5.8 #194

footballencarta · 2019-03-08T11:31:55Z

Currently, the package does not work with Laravel 5.8 due to changes to the testcase which now requires a return type on the setUp and tearDown methods.

This change constitutes test fixes for this change, as well as dropping support for the versions of PHP in the travis file that will not support this change (5.6 and 7.0) and adding support for the latest version (7.3).

I've also incorporated the changes from #193 and #191 .

As the PHP version is now specified in the composer.json - this change should not affect users of now unsupported versions of PHP - however it may be worth releasing as a new major version to stop any accidental upgrades

footballencarta · 2019-03-08T12:47:48Z

It looks like the scrutinizer job is configured for PHP 7.1, and ignores the platform requirements of the composer.json - meaning an invalid version of PHPUnit is installed.

Could you look at updating the version of PHP to 7.2 - or locking down the composer install os it doesn't ignore platform requirements.

Szasza · 2019-03-17T21:13:02Z

@baopham first of all thank you for the package.

Is there a chance to modify the Scrutinizer config, so we can get Laravel/Lumen 5.8 support without having to use patchy ways of pulling the package in?

Thank you in advance for your insights.

baopham · 2019-03-21T15:44:05Z

Sorry for the wait.

Updated the config. Do you want to rebase and push again to trigger a re-run?

baopham · 2019-03-21T15:49:01Z

.travis.yml

@@ -7,10 +7,9 @@ stages:
 - test

 php:
- - '5.6'
- - '7.0'


Did travis drop support for 5.6 and 7.0?

No, Laravel 5.8 did. PHP 5.6 and PHP 7.0 is EOL and doesn’t receive any updates anymore.

The new void return declarations with Laravel 5.8 will only work with PHP 7.1 and above.

baopham · 2019-03-21T15:54:00Z

composer.json

@@ -3,9 +3,10 @@
 "description": "Eloquent syntax for DynamoDB",
 "keywords": ["laravel", "dynamodb", "aws"],
 "require": {
+ "php": "^7.1.3",


What would happen if people are using Laravel 5.2 with older PHP version and they want to upgrade this library for bug fixes?

The semantic versioning in the composer.json should stop them from using the newer version. Hence the suggestion of a new major version to stop any accidental breaking changes.

I'd like them to be able to use the newer version of this library though because there are some bug fixes. We can improve on versioning later. Why do you think people using older PHP version should not upgrade this library?

Anyone that is using a currently supported version of PHP will be able to use this version. Laravel 5.8 requires PHP 7.1 and above - you can’t support both Laravel 5.8 and PHP versions less than 7.1.

I'm talking about people who are using Laravel 5.2 for example

Sounds like you need to start a new v5 release with this PR, while maintaining v4 with bugfixes only for older versions of Laravel. Old Laravel users can easily restrict this package to ^4.0 in their composer.json file. Right?

I understand the concerns about those older Laravel users. That said, there are a lot of us that really need to upgrade to Laravel 5.8 and this is a blocker.

@footballencarta You could remove the PHP version requirement here since Laravel will have already enforced that.

However, I see you've also added a bunch of PHP 7.1 specific code, like function return types. IMHO these are unnecessary and preventing this from getting merged quickly.

I would suggest backing off the PHP requirement (anyone installing this in Laravel 5.8 will have already dealt with Laravel's PHP 7.1 requirement) and removing your function return types.

A simple composer.json change to allow illuminate/support and illuminate/database at 5.8 will enable this package to be installed in Laravel 5.8 without breaking older versions as well. Seems like a simple and quick fix. Later if @baopham decides to have a separate major release for new versions of PHP & Laravel, PHP 7.1 specific function return types could be added.

...unless I'm missing something?

The return types are enforced by the version of PHPUnit that ships with Laravel 5.8 - in that it expects a return type to be added, otherwise it fails the entire test set for invalid code (PHPUnit 8+).

I understand the issue with supporting older versions - which is why all along I’ve suggest merging this into a new major version, as it contains breaking changes. This will allow the package to be developed to keep pace with Laravel’s standards, while also backporting big fixes to “legacy” versions of Laravel / PHP if needed.

Honestly, we’re in 2019 now, and PHP 7.1 has been around since the end of 2016. I get some people can’t upgrade for whatever reasons (hell we have some guys where I work that refuse to switch from 5.6) - but making this as a new major version supporting the next generation of Laravel applications is better than trying to maintain backwards compatibility with everything and eventually being left vastly far behind.

https://github.com/laravel/laravel/blob/master/composer.json#L22

https://github.com/laravel/framework/blob/master/composer.json#L88

I'm seeing PHPUnit 7.5 in laravel/laravel master, and either 7.5 or 8 in laravel/framework. Are you sure PHPUnit 8 is a hard requirement? Not seeing it.

If you follow @jszobody's suggestion, then we can go ahead with this PR. I don't think forcing the PHP version should be in this PR. Why do you need this line to unblock you?

crynobone · 2019-03-21T22:35:37Z

This package failed when running tests directly on Laravel 5.1

    "require-dev": {
        "laravel/framework": "~5.1.0",
        "phpunit/phpunit": "~4.0",
        "orchestra/testbench": "~3.0"
    },

crynobone · 2019-03-21T22:39:25Z

Also failed on Laravel 5.2

    "require-dev": {
        "laravel/framework": "~5.2.0",
        "phpunit/phpunit": "~4.0|~5.0",
        "orchestra/testbench": "~3.0"
    },

crynobone · 2019-03-21T22:41:47Z

Actual lowest working code is Laravel 5.3

dp88 · 2019-03-22T17:44:56Z

tests/DynamoDbClientServiceTest.php

@@ -13,7 +13,7 @@
 */
 class DynamoDbClientServiceTest extends TestCase
 {
- public function setUp()
+ public function setUp(): void


Are the type returns necessary? Would it be possible to remove these and drop the explicit php 7.1.3 requirement in composer.json? That could get us closer to the broadest range of Laravel compatibility.

They are required by PHPUnit 8

For development, I'm ok with the requirement of php7.1 and above.

Removed PHP version requirement.

footballencarta · 2019-03-22T18:43:06Z

I’ve removed the version requirement from the composer.json file.

baopham · 2019-03-22T18:43:49Z

Thanks! Fixing scrutinizer now. Will merge in a bit

scrutinizer-notifier · 2019-03-22T19:03:27Z

The inspection completed: No new issues

baopham · 2019-03-22T19:13:58Z

Thanks everyone for your effort, here's the new release including all the changes accumulated since.: https://github.com/baopham/laravel-dynamodb/releases/tag/4.12.0

Better version control is to come.

commit fe56587 Merge: 5f72c16 3833ac8 Author: Jack Price-Burns <JackPriceBurns@outlook.com> Date: Thu Feb 18 11:49:07 2021 +0000 Merge pull request baopham#234 from stancl/patch-1 Improve installation instructions commit 5f72c16 Merge: 2c7cba9 8827e04 Author: Jack Price-Burns <JackPriceBurns@outlook.com> Date: Thu Feb 18 11:46:46 2021 +0000 Merge pull request baopham#236 from xengulai/getClientDynamicConnectionName Allow connection name to be gathered dynamically commit 8827e04 Author: Jaysen Nuttall <j.nuttall@plansight.com> Date: Mon Feb 15 15:48:25 2021 -0700 Allow connection name to be gathered dynamically commit 3833ac8 Author: Samuel Štancl <samuel.stancl@gmail.com> Date: Fri Jan 8 20:03:30 2021 +0100 Improve installation instructions commit 2c7cba9 Merge: ae85c92 587ede8 Author: Jack Price-Burns <JackPriceBurns@outlook.com> Date: Wed Sep 30 11:45:19 2020 +0100 Merge pull request baopham#230 from baopham/hotfix/travis-fix Fix issues with travis ci commit 587ede8 Author: Jack Price-Burns <JackPriceBurns@outlook.com> Date: Wed Sep 30 11:38:37 2020 +0100 Fix issues with travis ci commit ae85c92 Merge: e5290d8 387a5e9 Author: Jack Price-Burns <JackPriceBurns@outlook.com> Date: Wed Sep 30 11:26:52 2020 +0100 Merge pull request baopham#225 from alfredkoncsag/patch-1 Update README.md commit e5290d8 Merge: 0d12b73 b9217ae Author: Jack Price-Burns <JackPriceBurns@outlook.com> Date: Wed Sep 30 11:22:56 2020 +0100 Merge pull request baopham#226 from fd-automox/7.4-fix Fix an invalid array access and enable CI for PHP 7.4 commit 0d12b73 Merge: cab7a95 57510e2 Author: Jack Price-Burns <JackPriceBurns@outlook.com> Date: Wed Sep 30 09:29:46 2020 +0100 Merge pull request baopham#227 from aislandener/patch-1 Documentation of Where clause with error commit cab7a95 Merge: 3c85a7b aaf8068 Author: Jack Price-Burns <JackPriceBurns@outlook.com> Date: Wed Sep 30 09:23:19 2020 +0100 Merge pull request baopham#229 from madeITBelgium/patch-1 Add support for Laravel 8 commit aaf8068 Author: Made I.T <info@madeit.be> Date: Tue Sep 15 22:24:24 2020 +0200 Add support for Laravel 8 commit 57510e2 Author: Aislan Dener Souza Vicentini <aislandenersouzavicentini@gmail.com> Date: Tue Aug 11 09:26:07 2020 -0300 Documentation of Where clause with error In the Where AND and OR documentation part there was no comma between the second and third parameters commit b9217ae Author: Fred Dysart <fred.dysart@automox.com> Date: Sat Aug 8 11:56:13 2020 -0600 Be consistent with how the type is accessed commit c054ebb Author: Fred Dysart <fred.dysart@automox.com> Date: Sat Aug 8 11:41:00 2020 -0600 Fix an invalid array access and enable CI for PHP 7.4 commit 387a5e9 Author: alfredkoncsag <koncsagalfred@gmail.com> Date: Thu Aug 6 12:25:03 2020 +0200 Update README.md Fix query index commit 3c85a7b Author: Pele <peleodiase@yahoo.co.uk> Date: Mon Mar 23 04:46:28 2020 +0000 Add support for Laravel 7 (baopham#218) commit a656c26 Author: Tudor-Dan Ravoiu <tudor2004@yahoo.com> Date: Mon Mar 23 06:46:07 2020 +0200 Fix PHP ^7.4 Notice: Trying to access array offset on value of type null (baopham#217) It seems that under PHP ^7.4 this check is throwing an notice when trying to run a query without using primary/composite key as filter attributes (so when trying to run a DynamoDB scan). This fix checks if the key is set and uses null otherwise. commit b442ad5 Author: Gustave P <56343436+guspio@users.noreply.github.com> Date: Sat Feb 1 05:01:31 2020 +0100 Changed version system for Laravel 6 (baopham#216) commit bc58f54 Author: Jack Price-Burns <JackPriceBurns@outlook.com> Date: Tue Jan 21 15:12:40 2020 +0000 Update readme to fix issue with facades (baopham#214) commit 6f16f9d Author: David Palmer <dp88@users.noreply.github.com> Date: Tue Jan 21 10:00:47 2020 -0500 Laravel 6.0 Support (baopham#203) * Laravel 6.0 Support * Remove PHP 7.1 Step from Travis Since PHP 7.1 will no longer be actively maintained after December of this year. commit 77239f9 Author: footballencarta <footballencarta@users.noreply.github.com> Date: Fri Mar 22 15:04:47 2019 -0400 Updated to run with Laravel 5.8 and fix tests (baopham#194) commit d080e78 Merge: af4f908 c17c2f9 Author: Bao Pham <gbaopham@gmail.com> Date: Thu Mar 21 23:28:10 2019 +0800 Merge pull request baopham#193 from sahilsharma011/patch-1 Update docs for find and findMany functions commit c17c2f9 Author: Sahil Sharma <sahilsharma011@users.noreply.github.com> Date: Fri Mar 8 15:37:16 2019 +0530 Update docs for find and findMany functions I wanted to use batchGetItem method using this package, but I was not able to find any function in docs which provided this functionality. When I went into the code, I found that there was already a function using batchGetItem method, but it was not listed in the docs. So I wanted to update the docs to make it clear. commit af4f908 Merge: 45bd597 05c0275 Author: Bao Pham <gbaopham@gmail.com> Date: Mon Feb 25 22:38:57 2019 +0800 Merge pull request baopham#190 from sahilsharma011/removeIsNotEmpty Change isNotEmpty to isEmpty commit 05c0275 Author: Sahil Sharma <sahilsharma011@gmail.com> Date: Mon Feb 25 14:43:06 2019 +0530 Change isNotEmpty to isEmpty commit 45bd597 Merge: 71aa7b5 a470c15 Author: Bao Pham <gbaopham@gmail.com> Date: Fri Feb 22 14:23:38 2019 +0800 Merge pull request baopham#187 from thomasedwards/patch-1 Adds note to README about SerializesModels on Jobs commit a470c15 Author: Thomas Edwards <thomas.edwards@gmail.com> Date: Mon Feb 18 12:06:52 2019 +0000 Adds spaces to EOL for Markdown formatting commit 37436a9 Author: Thomas Edwards <thomas.edwards@gmail.com> Date: Mon Feb 18 11:55:52 2019 +0000 Adds note to README about SerializesModels on Jobs commit 71aa7b5 Author: Matt Collins <mlcollins10@users.noreply.github.com> Date: Tue Feb 12 10:54:43 2019 -0700 Improve array of conditions support in DynamoDbQueryBuilder::where() (baopham#180)

Damon Williams added 3 commits March 8, 2019 11:13

Updated with changes for laravel 5.8

2f2e94e

Updated readme

fcf309f

Added PHP version to composer.json

680dcb9

footballencarta mentioned this pull request Mar 11, 2019

Fix broken tests #186

Closed

Szasza mentioned this pull request Mar 16, 2019

Lumen 5.8 Support - Dependencies required versions conflict #195

Closed

baopham mentioned this pull request Mar 21, 2019

Update composer.json for Laravel 5.8 support #191

Closed

baopham reviewed Mar 21, 2019

View reviewed changes

dp88 reviewed Mar 22, 2019

View reviewed changes

Update composer.json

8120ab7

Removed PHP version requirement.

baopham merged commit 77239f9 into baopham:master Mar 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated to run with Laravel 5.8 #194

Updated to run with Laravel 5.8 #194

footballencarta commented Mar 8, 2019

footballencarta commented Mar 8, 2019

Szasza commented Mar 17, 2019 •

edited

Loading

baopham commented Mar 21, 2019

baopham Mar 21, 2019

footballencarta Mar 21, 2019

baopham Mar 21, 2019

footballencarta Mar 21, 2019

baopham Mar 21, 2019

footballencarta Mar 21, 2019

baopham Mar 21, 2019

jszobody Mar 22, 2019

jszobody Mar 22, 2019

footballencarta Mar 22, 2019

jszobody Mar 22, 2019

baopham Mar 22, 2019

crynobone commented Mar 21, 2019 •

edited

Loading

crynobone commented Mar 21, 2019 •

edited

Loading

crynobone commented Mar 21, 2019

dp88 Mar 22, 2019

footballencarta Mar 22, 2019

baopham Mar 22, 2019

footballencarta commented Mar 22, 2019

baopham commented Mar 22, 2019

scrutinizer-notifier commented Mar 22, 2019

baopham commented Mar 22, 2019 •

edited

Loading

@@ @@ -7,10 +7,9 @@ stages: @@
  - test
 php:
- - '5.6'
- - '7.0'

Updated to run with Laravel 5.8 #194

Updated to run with Laravel 5.8 #194

Conversation

footballencarta commented Mar 8, 2019

footballencarta commented Mar 8, 2019

Szasza commented Mar 17, 2019 • edited Loading

baopham commented Mar 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crynobone commented Mar 21, 2019 • edited Loading

crynobone commented Mar 21, 2019 • edited Loading

crynobone commented Mar 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

footballencarta commented Mar 22, 2019

baopham commented Mar 22, 2019

scrutinizer-notifier commented Mar 22, 2019

baopham commented Mar 22, 2019 • edited Loading

Szasza commented Mar 17, 2019 •

edited

Loading

crynobone commented Mar 21, 2019 •

edited

Loading

crynobone commented Mar 21, 2019 •

edited

Loading

baopham commented Mar 22, 2019 •

edited

Loading