On 21-4-2022 21:56, Andreas Hennings wrote:
On Thu, 21 Apr 2022 at 11:18, Rowan Tommins <rowan.coll...@gmail.com> wrote:
On Wed, 20 Apr 2022 at 19:16, Claude Pache <claude.pa...@gmail.com> wrote:
You make a very important claim in your bug report:
However, in real world, is_callable() is almost never given totally
arbitrary stuff
My concern is that we have no way of knowing whether this is true, and
whether there will always be a clear action for users who see the
deprecation notice.
In this case, there is always the possibility for them to use the error
suppression operator. That remains easy, and looks like a reasonable use
case of that operator.
The error suppression operator is a big blunt tool, and using it to squash
a deprecation notice would be an absolute last resort. Note that in
contrast to the strpos() case where both behaviours can be achieved with
*permanent* changes, the error suppression would only be a temporary
workaround for error logging, with no value in itself.
If we have to balance between addressing the verified issue of not
emitting deprecation notice for well-known and fairly frequent code
patterns, and addressing the potential and non-fatal issue of emitting too
much notices for totally unknown use cases, the choice should be
straightforward.
Again, this is true IF you are correct that one set of patterns is common
and the other is not, but so far I've not seen any attempt to find out
whether that's true. The idea of ignoring "unknown" use cases seems really
dangerous - like closing your eyes so you can't see the bad effects of what
you do. What we need to know is whether such use cases are *rare*, or
whether the people using them are just not subscribed to this list.
One starting point would be to find usages in the top 100 or 1000 packages
on Packagist using https://github.com/nikic/popular-package-analysis -
perhaps Juliette has already done that when testing their PHPCompatibility
sniff?
As a package author, you might not really know where the value is coming from.
A parameter will be declared as `@param mixed $callback_or_not`, if it
internally calls the`is_callable()` method.
And a static analysis tool will only see `mixed`, not "mixed, but if
it is a string, it does not start with 'static::', or if i's an array,
the first value is not 'static'.".
On the other hand, the value probably won't be completely arbitrary.
It might be coming from a discovery process that reads yml files or
annotations, or that calls some kind of hook in php. Or it is
generated with string concatenation.
Whatever it is, it is probably explicitly or implicitly hard-coded somewhere.
Here is a starting point for a search. But only for explicit calls.
https://sourcegraph.com/search?q=context:global+is_callable%5C%28%28%5C%5B%27%28static%7Cself%29%27%7C%27%28static%7Cself%29::.*%27%29%2C+lang:php&patternType=regexp
I appreciate it very much that Claude is trying to move the issue I
raised forward.
Some responses to the discussion which has ensued:
1. In my opinion, the deprecation should only be shown when one of the
deprecated syntaxes is passed either to a function with a `callable`
type declaration or to `is_callable()`.
The deprecation notice should not be shown in any other circumstances,
so an arbitrary invalid value being passed to `is_callable()` should
still return `false` without deprecation notice.
2. While I do extensively test new PHPCompatibility sniffs, I have not
run the WIP sniff against the top 1000 projects from Packagist, nor do I
intend to or have the setup to do so.
I agree it would be a good idea to run a package analysis, but to be
fair, in all honesty that should have been done for the original RFC,
which was completely missing an impact analysis.
3. As for the pattern being common or not - the fact that I found it so
easily in multiple random projects which I elected to test the sniff
against, makes me believe the pattern is not _uncommon_.
I should also point out that all three of the projects which I
highlighted as examples in my previous response are used extensively,
the first two both have over 100 million downloads via Packagist, with
the first being a dependency for PHPUnit.
The third doesn't show anywhere near as many downloads, but is a
dependency of WordPress, which doesn't use Composer/Packagist for
distribution, but is - as we're all aware - widely used.
In case anyone is looking for them - these were the examples I posted
previously:
> Some examples:
> *
https://github.com/symfony/service-contracts/blob/bc0a2247c72d29241b5a06fb60dc1c9d9acf2a3a/ServiceSubscriberTrait.php#L39
> *
https://github.com/mockery/mockery/blob/c10a5f6e06fc2470ab1822fa13fa2a7380f8fbac/library/Mockery/Mock.php#L960
> *
https://github.com/simplepie/simplepie/blob/dacf0ed495d2e8fb306e526ca3f2a846af78a7c9/tests/oldtests/absolutize/RFC3986.5.4/base.php#L13
Kind regards,
Juliette
