Skip to content

debug #3

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

debug #3

wants to merge 1 commit into from

Conversation

chalasr
Copy link
Owner

@chalasr chalasr commented Jun 10, 2022

Q A
Branch? master for features / 3.4, 4.4, 5.0 or 5.1 for bug fixes
Bug fix? yes/no
New feature? yes/no
Deprecations? yes/no
Tickets Fix #...
License MIT
Doc PR symfony/symfony-docs#...

@chalasr chalasr closed this Jun 10, 2022
@chalasr chalasr deleted the debug-inf-loop branch June 10, 2022 11:31
chalasr pushed a commit that referenced this pull request Jul 22, 2022
@nicolas-grekas @jderusse @fabpot
Improve EntityValueResolver (#3)
5a3df5e 
Co-authored-by: Jérémy Derussé <[email protected]>
chalasr pushed a commit that referenced this pull request Jul 22, 2022
@fabpot
feature symfony#43854 [DoctrineBridge] Add an Entity Argument Resolve…
5675aa8 
…r (jderusse, nicolas-grekas)

This PR was merged into the 6.2 branch.

Discussion
----------

[DoctrineBridge] Add an Entity Argument Resolver

| Q             | A
| ------------- | ---
| Branch?       | 6.1
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | part of symfony#40333
| License       | MIT
| Doc PR        | todo

This PR provides an Argument Resolver for Doctrine entities.

This would replace the SensioFramework's DoctrineParamConverter (in fact most of the code is copy/pasted from here) and helps users to disable the paramConverter and fix the related issue.

usage:
```yaml
sensio_framework_extra:
    request:
        converters: false
services:
    Symfony\Bridge\Doctrine\ArgumentResolver\EntityValueResolver: ~
```

```php
#[Route('/blog/{slug}')]
public function show(Post $post)
{
}
```

or with custom options
```php
#[Route('/blog/{id}')]
public function show(
  #[MapEntity(entityManager: 'foo', expr: 'repository.findNotDeletedById(id)')]
  Post $post
)
{
}
```

Commits
-------

5a3df5e Improve EntityValueResolver (#3)
4524083 Add an Entity Argument Resolver
chalasr added a commit that referenced this pull request Nov 30, 2022
@chalasr
minor symfony#48384 [Console] Fix OutputInterface options int-mask …
258fb73 
…for PHPStan (ogizanagi)

This PR was merged into the 6.2 branch.

Discussion
----------

[Console] Fix `OutputInterface` options int-mask for PHPStan

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | N/A <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead -->
| License       | MIT
| Doc PR        | N/A

When upgrading an application to 6.2, I encountered this issue with PHPStan:

```shell
 ------ -------------------------------------------------------------------------------------------------------
  Line   Pilot/Runner/Output/MultiplexingOutput.php
 ------ -------------------------------------------------------------------------------------------------------
  79     Default value of the parameter #3 $options (0) of method
         App\Pilot\Runner\Output\MultiplexingOutput::write() is incompatible with type 1|2|4|16|32|64|128|256.
 ------ -------------------------------------------------------------------------------------------------------
```

The `MultiplexingOutput` implements the `OutputInterface` and defined `0` as the default value for `$options`:

```php
    public function write(string|iterable $messages, bool $newline = false, int $options = 0): void
```

as defined by the interface:

https://github.com/symfony/symfony/blob/69f46f231b16be1bce1e2ecfa7f9a1fb192cda22/src/Symfony/Component/Console/Output/OutputInterface.php#L40

When trying to use `self::OUTPUT_NORMAL | self::VERBOSITY_NORMAL` as default value:

```shell
 ------ -------------------------------------------------------------------------------------------------------
  Line   Pilot/Runner/Output/MultiplexingOutput.php
 ------ -------------------------------------------------------------------------------------------------------
  79     Default value of the parameter #3 $options (33) of method
         App\Pilot\Runner\Output\MultiplexingOutput::write() is incompatible with type 1|2|4|16|32|64|128|256.
 ------ -------------------------------------------------------------------------------------------------------
```

Or simply using `Output::write()` with specific options:

```shell
 ------ -------------------------------------------------------------------------------------------------------
  Line   Pilot/Runner/Output/MultiplexingOutput.php
 ------ -------------------------------------------------------------------------------------------------------
  81     Parameter #3 $options of method Symfony\Component\Console\Output\Output::write() expects
         1|2|4|16|32|64|128|256, 33 given.
 ------ -------------------------------------------------------------------------------------------------------
```

Using PHPStan's [`int-mask-of`](https://phpstan.org/writing-php-code/phpdoc-types#integer-masks) is the solution for this, and allows by nature the `0` value.
It was even used [at some point](symfony#47016 (comment)), but reverted to use `self::VERBOSITY_*|self::OUTPUT_*`. However, it does not behave as expected for masks.

So, either we use `int-mask-of`, or we revert to `int`?

Commits
-------

304a17a [Console] Fix OutputInterface options int-mask for PhpStan
chalasr pushed a commit that referenced this pull request Dec 16, 2022
@nicolas-grekas
bug symfony#48524 [HttpKernel] Fix CacheAttributeListener priority …
0f5a556 
…(HypeMC)

This PR was merged into the 6.2 branch.

Discussion
----------

[HttpKernel] Fix `CacheAttributeListener` priority

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Currently the `CacheAttributeListener` & the `IsGrantedAttributeListener` have the same priority:

```
Registered Listeners for "kernel.controller_arguments" Event
============================================================

 ------- --------------------------------------------------------------------------------------------------------- ----------
  Order   Callable                                                                                                  Priority
 ------- --------------------------------------------------------------------------------------------------------- ----------
  #1      Symfony\Component\HttpKernel\EventListener\CacheAttributeListener::onKernelControllerArguments()          10
  #2      Symfony\Component\Security\Http\EventListener\IsGrantedAttributeListener::onKernelControllerArguments()   10
  #3      Symfony\Component\HttpKernel\EventListener\ErrorListener::onControllerArguments()                         0
 ------- --------------------------------------------------------------------------------------------------------- ----------
```

Since the `CacheAttributeListener` is alphabetically first, it's first to get triggered. This can cause an unauthenticated user to receive a 304 Not modified instead of a 302 Redirect, resulting in the user seeing some stale content from when they were authenticated instead of getting redirected to the login page.

This PR changes the priority of the `CacheAttributeListener` to be lower than that of the `IsGrantedAttributeListener`.

Commits
-------

90eb89f [HttpKernel] Fix CacheAttributeListener priority
chalasr pushed a commit that referenced this pull request Dec 6, 2024
@nicolas-grekas
bug symfony#58921 [HttpKernel] Ensure HttpCache::getTraceKey() does…
32d55d7 
… not throw exception (lyrixx)

This PR was merged into the 5.4 branch.

Discussion
----------

[HttpKernel] Ensure `HttpCache::getTraceKey()` does not throw exception

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | -
| License       | MIT

We have such logs in our logs. It's in our raw PHP logs. They are not caught by monolog, it's too early

```
[11-Oct-2024 01:23:33 UTC] PHP Fatal error:  Uncaught Symfony\Component\HttpFoundation\Exception\SuspiciousOperationException: Invalid method override "__CONSTRUCT". in /var/www/redirection.io/backend/blue/vendor/symfony/http-foundation/Request.php:1234
Stack trace:
#0 /var/www/redirection.io/backend/blue/vendor/symfony/http-kernel/HttpCache/HttpCache.php(728): Symfony\Component\HttpFoundation\Request->getMethod()
#1 /var/www/redirection.io/backend/blue/vendor/symfony/http-kernel/HttpCache/HttpCache.php(207): Symfony\Component\HttpKernel\HttpCache\HttpCache->getTraceKey()
#2 /var/www/redirection.io/backend/blue/vendor/symfony/http-kernel/Kernel.php(188): Symfony\Component\HttpKernel\HttpCache\HttpCache->handle()
#3 /var/www/redirection.io/backend/blue/web/app.php(9): Symfony\Component\HttpKernel\Kernel->handle()
symfony#4 {main}
  thrown in /var/www/redirection.io/backend/blue/vendor/symfony/http-foundation/Request.php on line 1234

```

I managed to reproduced locally.
* Before the patch, without the http_cache, symfony returns a 405
* After the patch, without the http_cache, symfony returns a 405
* Before the patch, with the http_cache, symfony returns a 500, without any information (too early)
* After the patch, with the http_cache, symfony returns a 405

Commits
-------

a2ebbe0 [HttpKernel] Ensure HttpCache::getTraceKey() does not throw exception
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@chalasr