php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Request #79623 method_exists() is too strict in PHP 8
Submitted: 2020-05-24 09:59 UTC Modified: 2020-05-27 08:02 UTC
Votes:13
Avg. Score:4.5 ± 0.8
Reproduced:13 of 13 (100.0%)
Same Version:12 (92.3%)
Same OS:8 (61.5%)
From: nicolasgrekas@php.net Assigned: cmb (profile)
Status: Wont fix Package: Class/Object related
PHP Version: master-Git-2020-05-24 (Git) OS:
Private report: No CVE-ID: None
Welcome back! If you're the original bug submitter, here's where you can edit the bug or add additional notes.
If this is not your bug, you can add a comment by following this link.
If this is your bug, but you forgot your password, you can retrieve your password here.
Password:
Status:
Package:
Bug Type:
Summary:
From: nicolasgrekas@php.net
New email:
PHP Version: OS:

 

 [2020-05-24 09:59 UTC] nicolasgrekas@php.net
Description:
------------
While working on making Symfony compatible with PHP 8, we're noticing a lot of failure that look like:

TypeError: method_exists(): Argument #1 ($object_or_class) must be of type object|string, XXX given

See e.g. https://travis-ci.org/github/symfony/symfony/jobs/690567745

Can we relax method_exists() and make it accept any value as 1st arg?
None of the changes needed to remove these errors look valuable.
This would lower the cost of migrating to PHP 8 for the community at large.
(Symfony is just an early adopter here.)

Thanks for considering!


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2020-05-24 10:24 UTC] nikic@php.net
This change was originally made to align the behavior of method_exists() and proprety_exists(), where the latter already only accepted an object|string argument.

However, I have found this change to have an additional benefit after the fact: The issue is that method_exists() also accepts a string as first argument, and will treat it as a class name in that case, which involves invoking the autoloader. This means that if you have code like method_exists($arbitraryValue, 'method') you must make sure that at least !is_string($arbitraryValue) holds beforehand -- at which point you might as well just test for is_object($arbitraryValue). See https://github.com/guzzle/promises/pull/105/files for an instance of such a bug going unnoticed for a long time. This is a pretty severe issue (those autoloader invocations are definitely going to hurt performance, but may also impact security), but the previous behavior of the function made it very easy to make it, because it made it look like passing an arbitrary value to method_exists() is safe.

Now, looking at the particular warning in that travis log, I see that it is guarded by "!is_scalar($default) && !method_exists($default, '__toString')" and as such would not run into this issue. There's definitely false positives. But overall I still think that the tradeoff here is reasonable.
 [2020-05-24 10:56 UTC] nicolasgrekas@php.net
Thanks for the heads up.
For reference, here is the PR to fix the related failures in Symfony:
https://github.com/symfony/symfony/pull/36938

Let's close then.
 [2020-05-27 08:02 UTC] cmb@php.net
-Status: Open +Status: Wont fix -Package: *General Issues +Package: Class/Object related -Assigned To: +Assigned To: cmb
 [2020-05-27 08:02 UTC] cmb@php.net
> Let's close then.

Okay, fine.
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Wed Apr 24 21:01:29 2024 UTC