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

Fix error handler allowUndefinedVars #735

Closed
wants to merge 2 commits into from

Conversation

rudiedirkx
Copy link

handleError only checks for property value, which is odd. I'm getting error

Attempt to read property "id" on null

Because id is the property being accessed. So it should check for any property name, not just value.

@@ -66,7 +66,7 @@ public function deactivate() {
*/
public function handleError($errno, $errstr, $errfile, $errline, $errcontext = [])
{
if ($this->allowUndefinedVars && $errstr == 'Attempt to read property "value" on null') {
if ($this->allowUndefinedVars && preg_match('/^Attempt to read property ".+?" on null$/', $errstr)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to use \w instead of ., when we need to catch alphanumeric case.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Properties can also include - and maybe even : etc, so it uneagerly matches to the first ". https://3v4l.org/s9KPc

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense.

@Martijn-Bos
Copy link

Martijn-Bos commented Aug 9, 2022

This pull request would make it possible to migrate to newer version without a large amount of changes. Could this be merged?

@rudiedirkx
Copy link
Author

Does nobody have allowUndefinedVars enabled in PHP 8.0+?

@wisskid
Copy link
Contributor

wisskid commented Nov 22, 2022

This change seems straightforward. Should be able to merge this. Just to be clear: could you provide an exact issue? I.e. what problem does this solve?

@wisskid wisskid added the waiting Waiting for answer label Nov 23, 2022
@rudiedirkx
Copy link
Author

$smarty->assign('object', null);
====
{{$object->name}}

That would trigger a

Attempt to read property "name" on null


I'll fix the merge conflict.

@wisskid
Copy link
Contributor

wisskid commented Nov 24, 2022

I propose a slightly different implementation, see #832

@wisskid wisskid closed this Nov 24, 2022
@rudiedirkx rudiedirkx deleted the patch-1 branch November 24, 2022 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting Waiting for answer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants