alexpott
Repos
82
Followers
60
Following
2

Events

issue comment
[HttpFoundation] Ensure path info has a leading slash

@stof what do I need to do to get this one fixed?

Created at 1 day ago

Merge branch '6.0' into 6.1

  • 6.0: [Mime][TwigBridge] Fix tests

Merge branch '6.1' into 6.2

  • 6.1: [Mime][TwigBridge] Fix tests

[DependencyInjection] fix test

feature #46907 [Security] Add #[IsGranted()] (nicolas-grekas)

This PR was merged into the 6.2 branch.

Discussion

[Security] Add #[IsGranted()]

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

Extracted from #45415 (and modernized a lot).

I did not implement the proposals from Stof to keep this first iteration simple. I'd appreciate help to improve the attribute in a follow up PR :pray:

Commits

bf8d75ed86 [Security] Add #[IsGranted()]

[TwigBridge] Add #[Template()] to describe how to render arrays returned by controllers

feature #46906 [TwigBridge] Add #[Template()] to describe how to render arrays returned by controllers (nicolas-grekas)

This PR was merged into the 6.2 branch.

Discussion

[TwigBridge] Add #[Template()] to describe how to render arrays returned by controllers

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

Extracted from #45415 (and modernized a lot).

Unlike the@Template annotation, this attribute mandates a template name as argument. This removes the need for any template-guesser logic, making things explicit.

Using the attribute also turns FormInterface instances into FormView, adjusting the status code to 422 when a non-valid form is found in the parameters, similarly to what AbstractController::render() does.

Commits

c252606a4b [TwigBridge] Add #[Template()] to describe how to render arrays returned by controllers

Test _template attribute for Twig listener

[Security] Make #[IsGranted] final

minor #46915 [Security] Make #[IsGranted] final (chalasr)

This PR was merged into the 6.2 branch.

Discussion

[Security] Make #[IsGranted] final

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

Also make use of security-core domain's RuntimeException.

Commits

e1b50557b0 [Security] Make #[IsGranted] final

feature #46751 [VarExporter] Add trait to help implement lazy loading ghost objects (nicolas-grekas)

This PR was merged into the 6.2 branch.

Discussion

[VarExporter] Add trait to help implement lazy loading ghost objects

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

This PR packages an implementation of lazy loading ghost objects in a single LazyGhostObjectTrait (as a reminder, a lazy ghost object is an object that is created empty and that is able to initialize itself when being accessed for the first time.)

By using this trait, ppl can easily turn any existing classes into such ghost object implementations.

I target two use cases with this feature (but ppl are free to be more creative):

  1. lazy proxy generation for service containers;
  2. lazy proxy generation for entities.

In all cases, the generation itself is trivial using inheritance (sorry final classes.) For example, in order to turn a Foo class into a lazy ghost object, one just needs to do:

class FooGhost extends Foo implements LazyGhostObjectInterface
{
    use LazyGhostObjectTrait;
}

And then, one can instantiate ghost objects like this:

$fooGhost = FooGhost::createLazyGhostObject($initializer);

$initializer should be a closure that takes the ghost object instance as argument and initializes it. An initializer would typically call the constructor on the instance after resolving its dependencies:

$initializer = function ($instance) use ($etc) {
    // [...] use $etc to compute the $deps
    $instance->__construct(...$deps);
};

Interface LazyGhostObjectInterface is optional to get the behavior of a ghost object but gives a contract that allows managing them when needed:

    public function initializeLazyGhostObject(): void;
    public function resetLazyGhostObject(): bool;

Because initializers are not freed when initializing, it's possible to reset a ghost object to its uninitialized state. This comes with one limitation: resetting readonly properties is not allowed by the engine so these cannot be reset. The main target use case of this capability is Doctrine's EntityManager of course.

To work around the limitation with readonly properties, but also to allow creating partially initialized objects, $initializer can also accept two more arguments $propertyName and $propertyScope. When doing so, $initializer is going to be called on a property-by-property basis and is expected to return the computed value of the corresponding property.

Because lazy-initialization is not triggered when (un)setting a property, it's also possible to do partial initialization by calling setters on a just-created ghost object.


You might wonder why this PR is in the VarExporter component? The answer is that it reuses a lot of its existing code infrastructure. Exporting/hydrating/instantiating require using reflection a lot, and ghost objects too. We could consider renaming the component, but honestly, 1. I don't have a good name in mind; 2. changing the name of a component is costly for the community and 3. more importantly this doesn't really matter because this is all low-level stuff usually.

You might also wonder why this trait while we already have https://github.com/FriendsOfPHP/proxy-manager-lts and https://github.com/Ocramius/ProxyManager?

The reason is that the code infrastructure in ProxyManager is heavy. It comes with a dependency on https://github.com/laminas/laminas-code and it's complex to maintain. While I made the necessary changes to support PHP 8.1 in FriendsOfPHP/proxy-manager-lts (and submitted those changes upstream), getting support for new PHP versions is slow and complex. Don't take me wrong, I don't blame maintainers, ProxyManager is complex for a reason.

But ghost objects are way simpler than other kind of proxies that ProxyManager can produce: a trait does the job. While the trait itself is no trivial logic, it's at least plain PHP code, compared to convoluted (but needed) code generation logic in ProxyManager.

If you need any other kind of proxies that ProxyManager supports, just use ProxyManager.

For Symfony, having a simple lazy ghost object implementation will allow services declared as lazy to be actually lazy out of the box (today, you need to install proxy-manager-bridge as an optional dependency.) \o/

Commits

27b4325f78 [VarExporter] Add trait to help implement lazy loading ghost objects

[DependencyInjection] Use lazy-loading ghost object proxies out of the box

[HttpKernel] Add #[Cache] to describe the default HTTP cache headers on controllers

feature #46880 [HttpKernel] Add #[Cache()] to describe the default HTTP cache headers on controllers (nicolas-grekas)

This PR was merged into the 6.2 branch.

Discussion

[HttpKernel] Add #[Cache()] to describe the default HTTP cache headers on controllers

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

Extracted from #45415 (and modernized a lot).

I'd appreciate any help for porting the other attributes following this leading PR :pray:

Commits

acd4fa7d90 [HttpKernel] Add #[Cache] to describe the default HTTP cache headers on controllers

feature #46752 [DependencyInjection] Use lazy-loading ghost object proxies out of the box (nicolas-grekas)

This PR was merged into the 6.2 branch.

Discussion

[DependencyInjection] Use lazy-loading ghost object proxies out of the box

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

This PR builds on #46751. It also replaces #46458.

Instead of using ProxyManager to make lazy services actually lazy, using LazyGhostObjectTrait from #46751 allows doing so out of the box - aka without the need to install any optional dependencies.

When a virtual proxy is required (typically when using the proxy tag), ProxyManager is still required (and the dep remains optional.)

But for most services, using LazyGhostObjectTrait just works \o/

Commits

58a184826a [DependencyInjection] Use lazy-loading ghost object proxies out of the box

minor #46914 [TwigBridge]  Add test on _template attribute for Twig listener (GromNaN)

This PR was merged into the 6.2 branch.

Discussion

[TwigBridge]  Add test on _template attribute for Twig listener

| Q | A | ------------- | --- | Branch? | 6.2 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | Fix https://github.com/symfony/symfony/pull/46906#issuecomment-1181398235 | License | MIT | Doc PR | n/a

Add test for the feature described in https://github.com/symfony/symfony/pull/46906#issuecomment-1181398235

Commits

86cf8de367 Test _template attribute for Twig listener

fix sending request to paths containing multiple slashes

[Console] Be explicit about the completion API version

Using the application version is wrong, especially for standalone apps like PHPStan. The completion API is not bound to change on a PHPStan version upgrade, but instead only when something in symfony/console changes.

Check for null instead of type

minor #46913 Check for null instead of type (ihmels)

This PR was submitted for the 6.2 branch but it was merged into the 5.4 branch instead.

Discussion

Check for null instead of type

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

LogoutEvent::getResponse() returns a Response or null, so it is not necessary to check the type, but whether the value is null.

Commits

f545be9d6d Check for null instead of type

[DependencyInjection] Fix dumping lazy services with parametrized class

Created at 1 day ago
issue comment
Allow custom processing of WebAssert::assert through callback

Yes but doing that means re-throwing the exception from the onNotSuccessful and therefore changing the exception type. We have tried to propose changes to PHPUnit to facilitate this see https://github.com/sebastianbergmann/phpunit/issues/4983#issuecomment-1257202297

Atm if we want to mark Mink exceptions as PHPUnit failures we have do something like:


    if ($t instanceof MinkException) {
      $e = new AssertionFailedError($t->getMessage(), $t->getCode(), $t);
      $trace = $t->getTrace();
      foreach ($trace as $i => $call) {
        unset($trace[$i]['args']);
      }
      $reflection = new \ReflectionProperty($e, 'serializableTrace');
      $reflection->setValue($e, $trace);
      throw $e;
    }

in order to preserve the trace. Which in my mind not very sensible.

Created at 6 days ago
Allow specific exceptions to be marked as failures instead of errors

@sebastianbergmann thanks for thinking about the technical solution.

So while we can technically fix this in Drupal I think there is a question about whether this is the correct fix. Integrating PHPUnit with something like Mink that provides its own assertions is really useful. Having to use reflection to handle setting properties on exceptions feels like a not great solution. As an alternative is it possible to explore the question asked in the original comment:

Is it possible to add a feature to PHPUnit where specific classes of exceptions can be reported as failures?

Created at 2 months ago