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.

Sign up for GitHub

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

Jump to bottom

Updated to run with Laravel 5.8 #194

Merged
merged 4 commits into from
Mar 22, 2019
Merged

Updated to run with Laravel 5.8 #194

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2f2e94e
Updated with changes for laravel 5.8
Mar 8, 2019
fcf309f
Updated readme
Mar 8, 2019
680dcb9
Added PHP version to composer.json
Mar 8, 2019
8120ab7
Update composer.json
footballencarta Mar 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,9 @@ stages:
- test

php:
- '5.6'
- '7.0'
Copy link
Owner

Choose a reason for hiding this comment

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

Did travis drop support for 5.6 and 7.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

- '7.1'
- '7.2'
- '7.3'

before_script:
- java -Djava.library.path=./DynamoDBLocal_lib -jar dynamodb_local/DynamoDBLocal.jar --port 3000 &
Expand Down
4 changes: 3 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,8 @@ Usage
#### find() and delete()

```php
$model->find(<id>);
$model->find($id, array $columns = []);
$model->findMany($ids, array $columns = []);
$model->delete();
$model->deleteAsync()->wait();
```
Expand Down Expand Up @@ -607,3 +608,4 @@ Author and Contributors
* [Alexander Ward](https://github.com/cthos)
* [Quang Ngo](https://github.com/vanquang9387)
* [David Higgins](https://github.com/zoul0813)
* [Damon Williams](https://github.com/footballencarta)
6 changes: 3 additions & 3 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@
"description": "Eloquent syntax for DynamoDB",
"keywords": ["laravel", "dynamodb", "aws"],
"require": {
"php": "^7.1.3",
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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?

"aws/aws-sdk-php": "^3.0.0",
"illuminate/support": "5.1.* || 5.2.* || 5.3.* || 5.4.* || 5.5.* || 5.6.* || 5.7.*",
"illuminate/database": "5.1.* || 5.2.* || 5.3.* || 5.4.* || 5.5.* || 5.6.* || 5.7.*"
"illuminate/support": "5.1.* || 5.2.* || 5.3.* || 5.4.* || 5.5.* || 5.6.* || 5.7.* || 5.8.*",
"illuminate/database": "5.1.* || 5.2.* || 5.3.* || 5.4.* || 5.5.* || 5.6.* || 5.7.* || 5.8.*"
},
"license": "MIT",
"authors": [
Expand All @@ -19,7 +20,6 @@
"BaoPham\\DynamoDb\\": "src/"
}
},
"minimum-stability": "dev",
"require-dev": {
"orchestra/testbench": "~3.0"
},
Expand Down
2 changes: 2 additions & 0 deletions src/RawDynamoDbQuery.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,8 @@ public function count()
* For backward compatibility, previously we use array to represent the raw query
*
* @var array
*
* @return array
*/
private function internal()
{
Expand Down
2 changes: 1 addition & 1 deletion tests/DynamoDb/DynamoDbManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ class DynamoDbManagerTest extends DynamoDbTestCase
*/
protected $mockedClient;

public function setUp()
public function setUp(): void
{
parent::setUp();

Expand Down
4 changes: 2 additions & 2 deletions tests/DynamoDbClientServiceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
*/
class DynamoDbClientServiceTest extends TestCase
{
public function setUp()
public function setUp(): void
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are required by PHPUnit 8

Copy link
Owner

Choose a reason for hiding this comment

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

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

{
parent::setUp();

Expand Down Expand Up @@ -75,7 +75,7 @@ public function testUnsetDynamoDbClientService()
/**
* Make sure we are not leaving any values set on the DynamoDbModel
*/
public function tearDown()
public function tearDown(): void
{
parent::tearDown();

Expand Down
2 changes: 1 addition & 1 deletion tests/DynamoDbModelTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ abstract class DynamoDbModelTest extends DynamoDbTestCase
*/
protected $marshaler;

public function setUp()
public function setUp(): void
{
parent::setUp();

Expand Down
2 changes: 1 addition & 1 deletion tests/DynamoDbTestCase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
*/
abstract class DynamoDbTestCase extends TestCase
{
public function setUp()
public function setUp(): void
{
parent::setUp();

Expand Down
2 changes: 1 addition & 1 deletion tests/Parsers/ConditionExpressionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ class ConditionExpressionTest extends TestCase
*/
private $values;

public function setUp()
public function setUp(): void
{
parent::setUp();
$this->names = new ExpressionAttributeNames();
Expand Down
2 changes: 1 addition & 1 deletion tests/Parsers/UpdateExpressionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class UpdateExpressionTest extends TestCase
*/
private $names;

public function setUp()
public function setUp(): void
{
parent::setUp();
$this->names = new ExpressionAttributeNames();
Expand Down