Fix setting error_reporting #1675

carlos-granados · 2025-10-21T18:35:48Z

When the error reporting level was set (either by using withErrorReporting() or by using the default value of E_ALL), we were calling set_error_handler() with this level but were not calling error_reporting(). This meant that event though we had set a handler to be called on certain error levels, this handler might not be called if the error_reporting level set in php.ini was more restrictive. I saw this in a system where error_reporting was set to E_ALL & ~E_DEPRECATED & ~E_STRICT and the test With default error reporting reports all PHP errors and deprecations failed because the deprecation was not raised

This PR fixes this problem by actually setting the error_reporting level, not just using it in the set_error_handler call

Fixes #1663

features/error_reporting.feature

acoulton

@carlos-granados ahhh - that makes sense. Thanks for doing this, sorry I didn't get a chance to get back to it.

One tiny comment on the implementation.

I guess when this ships, people who were using their PHP configuration (rather than their Behat configuration) to e.g. ignore E_DEPRECATED may see failures.

However, I think that's acceptable if we highlight this in the changelog - it's better that we fix the issue that the ->withErrorReporting config doesn't currently work as expected.

acoulton · 2025-10-22T11:15:59Z

src/Behat/Testwork/Call/Handler/RuntimeCallHandler.php

+        $this->previousErrorReporting = error_reporting();
+        error_reporting($errorReporting);


Couldn't this just be a single call to error_reporting?

ah, yes, of course

carlos-granados · 2025-10-22T11:31:18Z

@acoulton I was already thinking about what you mentioned and I think it would be problematic to release the fix as it is because many people would not have set the error level using the config, so they may suddenly start seeing deprecations and other errors which are silenced by their php.ini config and I think that should be avoided.
So what I was thinking was:

If you set the error level using the config, use the fix and respect the configured value
If you don't set the error level using the config, go back to the previous behaviour where error reporting used the value defined in the ini config
What do you think?

acoulton · 2025-10-22T12:57:26Z

@carlos-granados I guess... The only thing is, can we tell whether the user has set the error_reporting in config?

Obviously we could add some state tracking to the ->withErrorReporting call in the PHP config interface, but if they're still using YAML that won't help.

I suppose we could change the default value from E_ALL to error_reporting() so that if the user doesn't explicitly configure it for Behat then it follows the system default?

Behat/src/Behat/Testwork/Call/ServiceContainer/CallExtension.php

Lines 69 to 71 in be3911b

    
           ->scalarNode('error_reporting') 
        
               ->info('Call executor will catch exceptions matching this level') 
        
               ->defaultValue(E_ALL)

And then we can use your patch as-is (even though the call to "change" the error_reporting level will essentially be a no-op).

Off the top of my head I think that would match the current behaviour if the user has not explicitly configured Behat specifically, while fixing the issue if they have?

carlos-granados · 2025-10-22T13:04:38Z

@carlos-granados I guess... The only thing is, can we tell whether the user has set the error_reporting in config?

Obviously we could add some state tracking to the ->withErrorReporting call in the PHP config interface, but if they're still using YAML that won't help.

I suppose we could change the default value from E_ALL to error_reporting() so that if the user doesn't explicitly configure it for Behat then it follows the system default?

Behat/src/Behat/Testwork/Call/ServiceContainer/CallExtension.php

Lines 69 to 71 in be3911b

->scalarNode('error_reporting')

->info('Call executor will catch exceptions matching this level')

->defaultValue(E_ALL)

And then we can use your patch as-is (even though the call to "change" the error_reporting level will essentially be a no-op).

Off the top of my head I think that would match the current behaviour if the user has not explicitly configured Behat specifically, while fixing the issue if they have?

Sounds like a good solution, let me test it. For now I am marking this PR as draft

carlos-granados · 2025-10-22T17:33:49Z

@acoulton updated with your suggestion and tests modified to check all needed cases

acoulton

Great, thanks @carlos-granados - just one small comment on BC

acoulton · 2025-10-24T07:52:04Z

src/Behat/Testwork/Call/Handler/RuntimeCallHandler.php

-        private $errorReportingLevel = E_ALL,
+        private $errorReportingLevel,


It could technically be a BC break to make this parameter non-optional?

I think we could safely leave it with the default E_ALL, if we know that in practice in our usage we're always providing a value?

Yeah, you are right, we don't really need to remove this default value to get this working. Restored

# Conflicts: # features/bootstrap/FeatureContext.php

acoulton

Perfect. Thanks for adding such comprehensive feature coverage :-)

carlos-granados · 2025-10-24T08:49:23Z

@acoulton perhaps the time for a new release?

acoulton · 2025-10-24T08:54:01Z

I was thinking I'd try and update/replace the two existing JUnit-related PRs (#1459 &#1601) before we release: possibly easier for users if all the changes to junit content happen in the same release?

Not sure if I'll get to those today, but hopefully by early next week (if not then we can ship as we are, I - don't think they're blocking, just nice-to-have).

carlos-granados · 2025-10-24T09:01:07Z

I

I was thinking I'd try and update/replace the two existing JUnit-related PRs (#1459 &#1601) before we release: possibly easier for users if all the changes to junit content happen in the same release?

Not sure if I'll get to those today, but hopefully by early next week (if not then we can ship as we are, I - don't think they're blocking, just nice-to-have).

If you want, I can work on the second one (#1601) as it is related to paths, where I have done a lot of work recently, and you can work on the first one which is more familiar to you

acoulton · 2025-10-24T10:30:08Z

If you have time that would be great :)

carlos-granados commented Oct 21, 2025

View reviewed changes

features/error_reporting.feature Show resolved Hide resolved

carlos-granados requested a review from acoulton October 21, 2025 19:06

acoulton reviewed Oct 22, 2025

View reviewed changes

carlos-granados marked this pull request as draft October 22, 2025 13:04

carlos-granados force-pushed the fix-error-reporting branch from 394fc6b to 6a4691a Compare October 22, 2025 17:22

carlos-granados marked this pull request as ready for review October 22, 2025 17:22

carlos-granados force-pushed the fix-error-reporting branch from 6a4691a to 98306a0 Compare October 22, 2025 17:27

carlos-granados requested a review from acoulton October 22, 2025 17:33

acoulton reviewed Oct 24, 2025

View reviewed changes

carlos-granados added 2 commits October 24, 2025 10:28

Fix setting error_reporting

0fa6d7e

# Conflicts: # features/bootstrap/FeatureContext.php

Restore default value in RuntimeCallHandler

fd48063

carlos-granados force-pushed the fix-error-reporting branch from 98306a0 to fd48063 Compare October 24, 2025 08:35

carlos-granados requested a review from acoulton October 24, 2025 08:40

acoulton approved these changes Oct 24, 2025

View reviewed changes

carlos-granados merged commit b759ab6 into Behat:master Oct 24, 2025
19 checks passed

carlos-granados deleted the fix-error-reporting branch October 24, 2025 08:49

		$this->previousErrorReporting = error_reporting();
		error_reporting($errorReporting);

		private $errorReportingLevel = E_ALL,
		private $errorReportingLevel,

Uh oh!

Fix setting error_reporting #1675

Fix setting error_reporting #1675

Uh oh!

Conversation

carlos-granados commented Oct 21, 2025

Uh oh!

Uh oh!

acoulton left a comment

Choose a reason for hiding this comment

Uh oh!

acoulton Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

carlos-granados Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

carlos-granados commented Oct 22, 2025

Uh oh!

acoulton commented Oct 22, 2025

Uh oh!

carlos-granados commented Oct 22, 2025

Uh oh!

carlos-granados commented Oct 22, 2025

Uh oh!

acoulton left a comment

Choose a reason for hiding this comment

Uh oh!

acoulton Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

carlos-granados Oct 24, 2025

Choose a reason for hiding this comment

Uh oh!

acoulton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carlos-granados commented Oct 24, 2025

Uh oh!

acoulton commented Oct 24, 2025

Uh oh!

carlos-granados commented Oct 24, 2025

Uh oh!

acoulton commented Oct 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants