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

SoftDelete trait throws PHP error with PHP7.2 #22162

Closed
EspadaV8 opened this issue Nov 22, 2017 · 14 comments
Closed

SoftDelete trait throws PHP error with PHP7.2 #22162

EspadaV8 opened this issue Nov 22, 2017 · 14 comments

Comments

@EspadaV8
Copy link
Contributor

  • Laravel Version: 5.1.46
  • PHP Version: 7.2.0RC6
  • Database Driver & Version: pgsql

Description:

Using the SoftDelete trait on a model when using PHP 7.2 throws an internal error because of the usage of count in the SoftDeletingScope on line 75. The property joins isn't always an array and results in a call of count(null).

Updating line 92 of Illuminate/Database/Query/Builder.php to

    /**
     * The table joins for the query.
     *
     * @var array
     */
    public $joins = [];

is enough to stop the error from happening, although it looks like there are multiple other properties in the class that should be updated in a similar way to prevent this error from happening elsewhere.

Steps To Reproduce:

  • Install a version of PHP 7.2
  • Create a model that implements SoftDeletes
  • Attempt to delete an instance of the model
  • Notice the error count(): Parameter must be an array or an object that implements Countable
@Dylan-DPC-zz
Copy link

Hey

5.1 is no longer supported. I submitted a PR fixing this for 5.6

@EspadaV8
Copy link
Contributor Author

I thought 5.1 had support for another year still (because of the LTS). 5.5 should also be updated too I think, again, because it's an LTS release.

@Dylan-DPC-zz
Copy link

Yeah but it can be considered a breaking change for 5.5

@EspadaV8
Copy link
Contributor Author

I don't see how it'd be a breaking change. All of the doc blocks there hint it as an array, it's just not been initialised correctly. If it is a breaking change then the call to count can be changed to do a check for null first (and revert the change initialising the array)

@paulofreitas
Copy link
Contributor

paulofreitas commented Nov 22, 2017

This shouldn't be affecting 5.5 for what I've tested. We have tests for soft deleting feature (e.g. DatabaseSoftDeletingScopeTest and DatabaseSoftDeletingTraitTest) and the Continuous Integration builds for PHP 7.2 aren't failing: https://travis-ci.org/laravel/framework/builds

Note that this same line of SoftDeletingScope has already been fixed: https://github.com/laravel/framework/blob/5.5/src/Illuminate/Database/Eloquent/SoftDeletingScope.php#L55 Going back in time we can see that this was fixed in 5.4 after merging PR #20258. 😉

This indeed affects Laravel 5.1, but the support for bug fixes have been ended - just security fixes are being accepted. 👍

@GrahamCampbell
Copy link
Member

Thanks everyone.

@assertchris
Copy link
Contributor

assertchris commented Nov 30, 2017

If 5.1.x is LTS and doesn't support 7.2 then the documentation should reflect that. This page only says PHP >= 5.5.9 (and not PHP >= 5.5.9 < 7.2.0) which means it is promising something that is not being delivered...

@Dylan-DPC-zz
Copy link

Yes it does because when 5.1 was created PHP 7.2 wasn't even though about 😛

@assertchris
Copy link
Contributor

assertchris commented Nov 30, 2017

I understand that. I'm not suggesting it should support 7.2. Just that releases still being supported should at least tell users which versions of PHP aren't supported. That's not unreasonable.

Edit: I'm happy to submit a PR if it'll help.

@odesenvolvedor
Copy link

I solved by changing "$originalWhereCount = count($query->wheres);" for "$originalWhereCount = is_null($query->wheres)
                     ? 0 : count($query->wheres);" in the function "callScope(callable $scope, $parameters = [])" in the file "/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Builder.php"

@fidahasan
Copy link

we are using Laravel 5.1.46 (LTS), PHP 7.2.4 (project is too large to upgrade)
any solution without changing core file/s ?

@EspadaV8
Copy link
Contributor Author

EspadaV8 commented Jun 9, 2018

You could see about extending the base classes and then using those throughout the project.

Since 5.1 is now EOL and won't get any new changes you could also fork the Laravel project and make the changes there, then update you composer.json to use your fork.

@fidahasan
Copy link

Thank You @EspadaV8
i haven't extended any base classes yet.. will try it.

@jciezaa
Copy link

jciezaa commented Aug 13, 2018

Thanks!!!

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

No branches or pull requests

8 participants