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

[11.x] Remove usage of compact() in Container #53789

Merged
merged 1 commit into from
Dec 8, 2024

Conversation

KennedyTedesco
Copy link
Contributor

This removes the only compact() function call in Container and replaces it with a direct array, saving a function call.

@taylorotwell taylorotwell merged commit 493a46c into laravel:11.x Dec 8, 2024
40 checks passed
@KennedyTedesco KennedyTedesco deleted the container branch December 9, 2024 18:52
browner12 pushed a commit to browner12/framework that referenced this pull request Dec 10, 2024
@andrey-helldar
Copy link
Contributor

Benchmark::dd([
    'compact' => function () {
        $foo = 'foo';

        $item = compact('foo');

        return $item['foo'];
    },
    'array'   => function () {
        $foo = 'foo';

        $item = ['foo' => $foo];

        return $item['foo'];
    },
], 10000);
$ art foo
array:2 [
  "compact" => "0.001ms"
  "array" => "0.001ms"
]

The compact function reads easier ¯_(ツ)_/¯

@KennedyTedesco
Copy link
Contributor Author

KennedyTedesco commented Dec 11, 2024

The performance difference is negligible (even though the array literal requires fewer opcodes to be executed).

Overall, I have the opposite opinion: I always prefer the array literal version because:

  • It's more explicit and easier to read (in my opinion).
  • It requires fewer opcode operations.
  • It's more IDE-friendly for static analysis and AST tools like Rector etc (even though static analyzers can handle the compact function).

I'm 100% in favor of using the array literal version, especially in this case where only two values are being handled.

@andrey-helldar
Copy link
Contributor

In this case, the phrase “all felt-tip pens are different for taste and color” applies 🙂

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.

3 participants