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

Multi-layer encoding #294

Open
theofidry opened this issue Oct 12, 2023 · 2 comments
Open

Multi-layer encoding #294

theofidry opened this issue Oct 12, 2023 · 2 comments
Assignees
Labels

Comments

@theofidry
Copy link

When calling assert like so:

Assert::count(..., message: $message);

$message is forwarded to sprintf. As a result if you are to do something like this:

Assert::count(
    ...,
    message: 'This is my custom message %s', // <- the placeholder will be filled up automatically, great!
);

Which does mean if you are using your own sprintf you should escape it first:

Assert::count(
    ...,
    message: 'This is my %%w custom message %s', // <- manually escape "%w" as this would make sprintf fail (it is an invalid format specifier)
);

// Automatic escaping:

Assert::count(
    ...,
    message: str_replace('%', '%%', 'This is my %%w custom message %s'),
);

So far so good. Now the problem: this does not work for all assertions. Indeed the example above with Assert::count() will still result in ValueError: Unknown format specifier "w".

The root of the issue is that an assertion can rely on other assertions internally, e.g. ::count() calls ::eq(). So the custom escaped message becomes unescaped when passing it to ::eq(). Making it work requires the user to find out how many items the string is evaluated to escape it accordingly (in this example twice).

I think this is a design flaw and the way it could be fixed is to escape the message passed within Assert when relying on inner assertions:

// Assert.php

public static function count($array, $number, $message = '')
    {
        static::eq(
            \count($array),
            $number,
            \sprintf(
                self::sprintfEscape($message) ?: 'Expected an array to contain %d elements. Got: %d.',
                $number,
                \count($array)
            )
        );
    }
    
    public static function sprintfEscape(string $value): string {
      return str_replace('%', '%%', $value);
    }
@shadowhand
Copy link
Collaborator

Will need to think about this one some more. I use a variety of custom messages, but generally I format the custom message before passing it to Assert::foo.

Maybe #221 would be a better solution to this?

@shadowhand shadowhand self-assigned this Oct 15, 2024
@theofidry
Copy link
Author

@shadowhand I think those are different problems entirely, but I am certainly in support of #221.

generally I format the custom message before passing it to Assert::foo

I do as well from time to time, but I don't like to figure out the "make mixed into a human readable string" logic which the library already does internally. So leveraging the placeholder is a nice feature

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

No branches or pull requests

2 participants