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

Incorrect behavior of PropertyAccessor when dynamic property contains dot #41845

Closed
kefzce opened this issue Jun 24, 2021 · 10 comments · Fixed by #49331
Closed

Incorrect behavior of PropertyAccessor when dynamic property contains dot #41845

kefzce opened this issue Jun 24, 2021 · 10 comments · Fixed by #49331

Comments

@kefzce
Copy link

kefzce commented Jun 24, 2021

Symfony version(s) affected: 5.2.0

Description
Attempting to serialize an array which casted to object, if any of these properties contains "." (dot)
PropertyAccess resolvePath(getPropertyPath) behaves weird

// first element is evaluated differently - no leading dot for properties
$pattern = '/^(([^\.\[]++)|\[([^\]]++)\])(.*)/';

How to reproduce

    protected function execute(InputInterface $input, OutputInterface $output): int
    {
        $arr = [
          "No. of Rows" => "somedata"
        ];

        $obj = (object) $arr;
        dump($obj->{"No. of Rows"}); // some data output
        $propertyAccessor = PropertyAccess::createPropertyAccessorBuilder()
            ->getPropertyAccessor();

        // instead of throwing an exception the following code returns null
        // in case if we decide to use disableExceptionOnInvalidPropertyPath
        $value = $propertyAccessor->getValue($obj, 'No. of Rows'); //exception
        dump($value);


        return Command::SUCCESS;
    }

Exception will be thrown, which we can handle by using disableExceptionOnInvalidPropertyPath
if we are using PropertyAccessor directly,
but not in case when we are using ObjectNormalizer / Serializer e.g $this->json($object)
propertyPath
resolve it as two elements

elements = {array} [2]
 0 = "No"
 1 = " Of Rows"

and of course an attempt to access the object with any of these properties
e.g

$obj->{"No"}
$obj->{" Of Rows"}

makes no sense, cause $zval does not know anything about these properties, but knows about No. of Rows
Possible Solution
Process this case correctly :D

Additional context
The example above was reproduced with simple test command in CLI environment

@sstok
Copy link
Contributor

sstok commented Jul 4, 2021

You can try using the PropertyPathBuilder with explicitly calling appendProperty().
The property-path string currently doesn't support special-case names.

Status: Reviewed

Or you can try casting an object to an array instead? And use an index-path.

@kefzce
Copy link
Author

kefzce commented Jul 4, 2021

You can try using the PropertyPathBuilder with explicitly calling appendProperty().
The property-path string currently doesn't support special-case names.

Status: Reviewed

Or you can try casting an object to an array instead? And use an index-path.

Is it possible to do it, when I'm using Serializer directly?
My case was about an exception during serialization

$this->json( (object) $myArray) );

@sstok
Copy link
Contributor

sstok commented Jul 4, 2021

The ObjectNormalizer allows to pass a PropertyAccessor instance, unless there is a Framework configuration (I haven't used the serializer myself) you can overwrite (decorate) the ObjectNormalizer service with a custom PropertyAccessor service.

Or we could change the PropertyPath parsing logic to handle special cases like
"No . way anyone ""Would"" do "" this".'[right?]'" (using quote doubling as escaping). As there are more special cases that could conflict.

But before we move forward with this idea I think it's best to hear from others 😄 technically it's possible 👍🏼

@carsonbot
Copy link

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

@carsonbot
Copy link

Could I get a reply or should I close this?

@carsonbot
Copy link

Hey,

I didn't hear anything so I'm going to close it. Feel free to comment if this is still relevant, I can always reopen!

@kefzce
Copy link
Author

kefzce commented Feb 7, 2022

Hey,

I didn't hear anything so I'm going to close it. Feel free to comment if this is still relevant, I can always reopen!

Could we open it once again ? I guess if no one will try to fix it, ill do it at my own.

@ogizanagi ogizanagi reopened this Feb 7, 2022
@ogizanagi ogizanagi removed the Stalled label Feb 7, 2022
@carsonbot
Copy link

Hey, thanks for your report!
There has not been a lot of activity here for a while. Is this bug still relevant? Have you managed to find a workaround?

@carsonbot
Copy link

Friendly ping? Should this still be open? I will close if I don't hear anything.

@carsonbot
Copy link

Hey,

I didn't hear anything so I'm going to close it. Feel free to comment if this is still relevant, I can always reopen!

chalasr added a commit that referenced this issue Feb 18, 2023
…oulain)

This PR was merged into the 6.3 branch.

Discussion
----------

[PropertyAccess] Allow escaping in PropertyPath

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Fix #41845
| License       | MIT
| Doc PR        | TODO symfony/symfony-docs#...

I'm not sure if it can be considered a feature or a fix.

Currently there is no way to escape `.` and `[` when using `PropertyPath`.
It can cause issue when we want to access a "property" containing a dot (for instance a key in an array).

I tried to modify the regexp as little as possible and to handle (really) edge cases such as double escaping.

Initially I had the issue in some Behat tests where I needed to evaluate some paths in a big JSON file (OpenAPI documentation).
Some keys contain dots and I cannot test the JSON node when it's the case.

Commits
-------

d534287 feat(property-access): allow escaping in PropertyPath
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants