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

Add error context #17976

Open
wants to merge 5 commits into
base: 5.x
Choose a base branch
from
Open

Add error context #17976

wants to merge 5 commits into from

Conversation

Harfusha
Copy link
Contributor

@ADmad
Copy link
Member

ADmad commented Oct 16, 2024

Can you give an e.g. of error message without file and line number?

I already see them in the error messages in the log file, for e.g.:

<date-here> warning: Could not render cell - Class `App\View\Cell\NavCell` does not have a `foo` method. [<redacted>/vendor/cakephp/cakephp/src/View/Cell.php, line 178]

@Harfusha
Copy link
Contributor Author

Old:

2024-10-17 05:33:43 warning: A warning caught by ErrorTrap
Request URL: /cart/calculator
Referer URL: ___
Client IP: ___
Trace:
/var/www/adam/dev/src/Controller/AppController.php /var/www/adam/dev/src/Controller/AppController.php, line 141
...

New:

2024-10-17 05:33:50 warning: A warning caught by ErrorTrap in /var/www/adam/dev/src/Controller/AppController.php on line 141
Stack Trace:
- /var/www/adam/dev/src/Controller/AppController.php:141
...
- [main]:

Request URL: /
Client IP: ___

Triggered by:
trigger_error("A warning caught by ErrorTrap", E_USER_WARNING);

@Harfusha
Copy link
Contributor Author

This was inconsistent between the log messages

@ADmad
Copy link
Member

ADmad commented Oct 17, 2024

So you are directly using the PHP function instead of the Cake function triggerWarning() which already includes the file and line number. Your change will duplicate that info for calls to triggerWarning() which isn't acceptable either.

@Harfusha
Copy link
Contributor Author

Not me, but libs i use.

  • This will make it same as normal warnings:
2024-10-17 06:35:00 error: [TypeError] in_array(): Argument #2 ($haystack) must be of type array, string given in /var/www/adam/dev/src/Controller/AppController.php on line 141
Stack Trace:
- /var/www/adam/dev/src/Controller/AppController.php:141
...
- [main]:

Request URL: /
Client IP: ___

Will add fix for the duplicity

@Harfusha
Copy link
Contributor Author

Harfusha commented Oct 17, 2024

Oh i see the problem. Each trigger_error used in cakephp has the file and line added where they are called.

I would like to make some function that will format it same way (When it is called or when it is outside cakephp, in ErrorTrap), so it can be parsed programatically. Now each type of error has own format.

Any ideas?

@markstory
Copy link
Member

I would like to make some function that will format it same way (When it is called or when it is outside cakephp, in ErrorTrap), so it can be parsed programatically. Now each type of error has own format.

The file/line could be removed from triggerWarning() and included in the output generated by ErrorTrap. The logic in triggerWarning() predates ErrorTrap by quite a bit of time, and we should be able to include file/line in rendered output more consistently now.

@dereuromark
Copy link
Member

Sounds like this should go into 5.next then, right? With such changes.

@ADmad
Copy link
Member

ADmad commented Oct 30, 2024

This doesn't change the public API and making the log output consistent is more of a bigfix than a new feature.

@Harfusha
Copy link
Contributor Author

I will try to make it next week :)

@dereuromark
Copy link
Member

dereuromark commented Nov 8, 2024

@Harfusha The next patch release is imminent in case you want to fit it into that one still.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants