Too few arguments to function ColinODell\Json5\SyntaxError::__construct()

[marc-farre]

Detailed description

Create this code:

// JSON5 code with a syntax error error, e.g. a single \ instead of a double \\
$jsonConfig = '{
  aaa: 'bbb\ccc',
}';

try {
            return Json5Decoder::decode($jsonConfig, true);
        } catch (SyntaxError $e) {
            throw new SyntaxError('Invalid JSON5 syntax in configuration file: ' . $e->getMessage());
        }

Result:

Too few arguments to function ColinODell\Json5\SyntaxError::__construct()

The problem is here: https://github.com/colinodell/json5/blob/main/src/SyntaxError.php#L33

Because $linenumber and $columnNumber are not provided.

Your environment

Library v2.3.0
PHP v8.2

[colinodell]

I think the problem is that your catch block is missing the required parameters. You will need to provide those if you wish to construct your own instance of that expectation class.

[marc-farre]

@colinodell thanks. Yes, you're right.
But the required parameters are $linenumber and $columnNumber.
And we cannot know the value of them.
Moreover, it's quite unusual having to pass 2 other parameters to an Exception.

Passing null as parameters works: throw new SyntaxError('Invalid JSON5 syntax in configuration file: ' . $e->getMessage(), null, null);

But I suggest, on line https://github.com/colinodell/json5/blob/main/src/SyntaxError.php#L33, replacing:

public function __construct($message, $linenumber, $columnNumber, $previous = null)

With:

public function __construct($message, $linenumber = null, $columnNumber = null, $previous = null)

[colinodell]

And we cannot know the value of them.

This library always sets those values: https://github.com/colinodell/json5/blob/v2.3.0/src/Json5Decoder.php#L604

If you want to set a custom message you have three options:

1. Use the values from the original exception: throw new SyntaxError('your message', $e->getLineNumber(), $e->getColumn()
2. Throw a more generic JsonException instead (potentially with the original exception as the $previous one)
3. Throw a different type of exception - perhaps your own custom InvalidConfigurationException

Moreover, it's quite unusual having to pass 2 other parameters to an Exception.

Passing null as parameters works:

Those parameters are required so we can guarantee that getLineNumber() and getColumn() always return integers (as documented in their docblocks). Passing null despite this (or changing the exception to allow null) would break this guarantee, and IMO a syntax error that doesn't point to a specific location isn't that useful. (Imagine if your PHP code had a syntax error but neither PHP nor your editor told you where the problem was)

So while having additional required parameters may be uncommon, it's not unprecedented (example), and I think it makes sense here.

[marc-farre]

@colinodell Thanks very much for your time and the explanations.
It's very clear and I agree, it's perfect the way it is implemented.

So for others, I update my code with a working one:

// JSON5 code with a syntax error error, e.g. a single \ instead of a double \\
$jsonConfig = '{
  aaa: 'bbb\ccc',
}';

try {
    return Json5Decoder::decode($jsonConfig, true);
} catch (SyntaxError $e) {
    throw new SyntaxError('Invalid JSON5 syntax in configuration file: ' . $e->getMessage(), $e->getLineNumber(), $e->getColumn());
}