laminas / laminas-developer-tools

Module for developer and debug tools for use with laminas-mvc applications.
BSD 3-Clause "New" or "Revised" License
24 stars 13 forks source link

Fix access of undefined array index file when internally analyzing stack frames #50

Closed reinfi closed 1 year ago

reinfi commented 2 years ago
Q A
Documentation no
Bugfix yes
BC Break no
New Feature no
RFC no
QA no

Description

When triggering events from within a controller plugin in laminas mvc the index is not a file because it is an invokable function.

4 = {array} [5] function = "__invoke" class = "Application\Controller\Plugin\ForwardToPlugin" object = {Application\Controller\Plugin\ForwardToPlugin} [3] type = "->" args = {array} [2]

I just checked if the index exists and return an empty string.

reinfi commented 1 year ago

Needs test additions

Yeah would love to add a test but it is quite difficult because there are no existing tests for the class and mocking the native buildin functions is quite hard.

Any suggestions how to start this?

Ocramius commented 1 year ago

I think a backtrace is not available when the code is either internal, or eval()'d?

A test could do something like:

$provider = new EventContextProvider();

$file = eval(<<<'PHP'
return (function (): void {
    return $provider->getEventTriggerFile();
})();
PHP
);

self::assertSame('', $file);

Alternatively using array_map() could perhaps get the same result.

reinfi commented 1 year ago

I added a crazy testcase for this specific case 🙈

Ocramius commented 1 year ago

Test case makes sense, now we need to make CI green :D

Also: 100% sure this makes PHPUnit fail when your patch is not applied?

samsonasik commented 1 year ago

Target branch need to be changed to 2.7.x or 2.8.x and rebase.

froschdesign commented 1 year ago

Target branch need to be changed to 2.7.x or 2.8.x and rebase.

A bugfix goes to the current release branch (2.7.x):

What branch to issue the pull request against?

Which branch should you issue a pull request against?

  • For fixes against the stable release, issue the pull request against the release branch matching the minor release you want to fix.
  • For new features, or fixes that introduce new elements to the public API (such as new public methods or properties), issue the pull request against the default branch.

https://github.com/laminas/.github/blob/a1debce9a0842e3000ae9e01fbc60e32db62901d/CONTRIBUTING.md#what-branch-to-issue-the-pull-request-against

reinfi commented 1 year ago

Test case makes sense, now we need to make CI green :D

Also: 100% sure this makes PHPUnit fail when your patch is not applied?

Undefined array key "file" /Users/maddin/sites/laminas-developer-tools/src/EventLogging/EventContextProvider.php:124

Happens if I remove my patch.

Target branch need to be changed to 2.7.x or 2.8.x and rebase.

A bugfix goes to the current release branch (2.7.x):

What branch to issue the pull request against?

Which branch should you issue a pull request against?

  • For fixes against the stable release, issue the pull request against the release branch matching the minor release you want to fix.
  • For new features, or fixes that introduce new elements to the public API (such as new public methods or properties), issue the pull request against the default branch.

https://github.com/laminas/.github/blob/a1debce9a0842e3000ae9e01fbc60e32db62901d/CONTRIBUTING.md#what-branch-to-issue-the-pull-request-against

Will try to solve this as best as possible :-D

reinfi commented 1 year ago

If I try to fix psalm and add use function call_user_func; then the test case fails again because there is no more a inline call for whatever reason.

I think I'll just close this cause creating a test case for it is quite annoying 🙈

reinfi commented 1 year ago

Okay I think I'll not be able to solve the psalm errors without breaking the test case again. I'll get some sleep and may be next week I have a solution for the testcase 👼

Ocramius commented 1 year ago

Hmm, something went wrong with the rebase here - I can take over, if it's not clear :)

reinfi commented 1 year ago

was too late yesterday 🙈 Would be happy if you help cause I got somewhere stuck with the rebase, tought it was right but it seems not.

Ocramius commented 1 year ago

Currently checking this locally: interestingly, if use function call_user_func; is used, then PHP optimizes the call away, and the test no longer works as expected :D

Ocramius commented 1 year ago

Rewriting some of the test to use array_map() fixes that though :+1:

Ocramius commented 1 year ago

I rewrote most of the patch, and GIT somehow lostsome ownership of the patch here, sorry :-(

Still, final result is much cleaner, I'd say.

Ocramius commented 1 year ago

Thanks @reinfi!

froschdesign commented 1 year ago

@reinfi Thank you for your time and this contribution! And also for your library reinfi/zf-dependency-injection 👍🏻

reinfi commented 1 year ago

thank you guys <3