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

feat: Use PropertyAccess Component syntax for nested variables access #302

Conversation

TheoD02
Copy link
Contributor

@TheoD02 TheoD02 commented Mar 1, 2024

Description

This pull request introduces a feature enhancement that allows the use of the Property Access Component syntax for accessing nested variables within the context.

Changes Made

  • Added the capability to utilize Property Access Component syntax for retrieving variables from the context.
  • Modified the .gitignore file to exclude the .idea/ directory.
  • Improved the example usage by replacing try/catch with the null coalescing operator for enhanced readability.
  • Upgraded the friendsofphp/php-cs-fixer dependency to version 3.51.0 to align the local and CI environments.

Example of Usage

// Context
new Context([
    'nested' => [
        'key' => 'nested_value',
    ],
]);

// Getting nested values
$foobar = variable('[foo][bar][baz]', 'default value');

// Same as:
$foobar = $context['foo']['bar']['baz'] ?? 'default value';

Warning

Not sure about the usage of the str_starts_with('[') condition for detecting when use PropertyAccess.

if (str_starts_with($key, '[')) {
    return PropertyAccess::createPropertyAccessor()->getValue($context, $key) ?? $default;
}

@TheoD02 TheoD02 force-pushed the feat/add_property_access_for_context_variable branch 2 times, most recently from cf1809f to 11c5955 Compare March 1, 2024 20:52
@TheoD02 TheoD02 force-pushed the feat/add_property_access_for_context_variable branch from 11c5955 to 673c035 Compare March 1, 2024 20:54
@@ -5,3 +5,4 @@
/my-app.*
/var/
/vendor/
.idea/
Copy link
Member

Choose a reason for hiding this comment

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

Could you revert that? It must be in your global git ignore

https://stackoverflow.com/a/7335487/685587

Comment on lines +118 to +119
if (str_starts_with($key, '[')) {
return PropertyAccess::createPropertyAccessor()->getValue($context, $key) ?? $default;
Copy link
Member

Choose a reason for hiding this comment

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

Indeed this is a bit weird. Could we find a way to always use property access?

As a user, I would expect:

$context = ['foo' => 'bar', 'nested' => ['a' => 'b']];
$foo = variable('bar');
$a = variable('nested['a']);

But When I write this, I really wonder why it's better than

$a = variable('nested')['a']

(Note: I'm not against this PR. Just asking)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but the component syntax doesn't allow you to do that, as far as I know. An element of an array must be enclosed in square brackets like [nested][a]

The only thing that's better than syntax two (current), is that the default value must be managed again in the case, for example, that the index doesn't exist.

$a = variable('nested')['a'] ?? null

And worse if null is not the default value

$a = variable('nested', 'another')['a'] ?? 'another'

But as indicated in the comment here #302 (comment), this approach could be handled in pure PHP without any deps

Copy link
Member

@pyrech pyrech left a comment

Choose a reason for hiding this comment

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

I'm not a huge fan of this PR as it adds complexity and a dependency for something already feasible in pure PHP (and so, a syntax more usual for PHP developers)

@TheoD02
Copy link
Contributor Author

TheoD02 commented Mar 4, 2024

I'm not a huge fan of this PR as it adds complexity and a dependency for something already feasible in pure PHP (and so, a syntax more usual for PHP developers)

That is true, this is mainly for the DX, WDYT about using this instead of PropertyAccess ?

$array = [
    'nested' => [
        'key' => 'nested_value',
    ],
];

$value = variable('nested.key', 'default_value'); // 'nested_value'

$value = variable('nested.key.unknown', 'default_value'); // 'default_value'

Note

Users who use . in array key will not benefit of this syntax

@lyrixx
Copy link
Member

lyrixx commented Mar 4, 2024

IMHO, you should not spend more time on this one. I don't really think the DX is better. I always feels awkward the "property access style" in tools.

More over, it breaks static code analyzer

@TheoD02
Copy link
Contributor Author

TheoD02 commented Mar 4, 2024

Okay, thanks for your feedbacks and time 😃

@TheoD02 TheoD02 closed this Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants