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

Drop useless call to end method #1359

Merged
merged 1 commit into from
Sep 24, 2024
Merged

Conversation

ddziaduch
Copy link
Contributor

Due to this, upgrading from SF 6.4.7 to more recent versions is impossible.

Output from the cache:clear after upgrade:

Executing script cache:clear [KO]
 [KO]
Script cache:clear returned with error code 255
!!  TypeError {#5946
!!    #message: "Symfony\Component\Config\Definition\Builder\NodeDefinition::end(): Return value must be of type Symfony\Component\Config\Definition\Builder\NodeParentInterface, null returned"
!!    #code: 0
!!    #file: "./vendor/symfony/config/Definition/Builder/NodeDefinition.php"
!!    #line: 98
!!    trace: {
!!      ./vendor/symfony/config/Definition/Builder/NodeDefinition.php:98 { …}
!!      ./vendor/enqueue/enqueue/Symfony/Client/DependencyInjection/ClientFactory.php:82 { …}
!!      ./vendor/enqueue/enqueue-bundle/DependencyInjection/Configuration.php:42 { …}
!!      ./vendor/symfony/config/Definition/Processor.php:46 { …}
!!      ./vendor/symfony/dependency-injection/Extension/Extension.php:109 { …}
!!      ./vendor/enqueue/enqueue-bundle/DependencyInjection/EnqueueExtension.php:34 { …}
!!      ./vendor/symfony/dependency-injection/Compiler/MergeExtensionConfigurationPass.php:76 { …}
!!      ./vendor/symfony/http-kernel/DependencyInjection/MergeExtensionConfigurationPass.php:42 { …}
!!      ./vendor/symfony/dependency-injection/Compiler/Compiler.php:80 { …}
!!      ./vendor/symfony/dependency-injection/ContainerBuilder.php:767 { …}
!!      ./vendor/symfony/http-kernel/Kernel.php:506 { …}
!!      ./vendor/symfony/http-kernel/Kernel.php:771 { …}
!!      ./vendor/symfony/http-kernel/Kernel.php:126 { …}
!!      ./vendor/symfony/framework-bundle/Console/Application.php:190 { …}
!!      ./vendor/symfony/framework-bundle/Console/Application.php:72 { …}
!!      ./vendor/symfony/console/Application.php:175 { …}
!!      ./bin/console:43 {
!!        › $application = new Application($kernel);
!!        › $application->run($input);
!!        › 
!!      }
!!    }
!!  }
!!  2024-09-17T12:45:24+00:00 [critical] Uncaught Error: Symfony\Component\Config\Definition\Builder\NodeDefinition::end(): Return value must be of type Symfony\Component\Config\Definition\Builder\NodeParentInterface, null returned
!!  
Script @auto-scripts was called via post-install-cmd

Due to this, upgrading from SF 6.4.7 to more recent versions is impossible.
@ddziaduch
Copy link
Contributor Author

@makasim @p-pichet can this be merged? It is quite important for us

@makasim
Copy link
Member

makasim commented Sep 19, 2024

sorry, I might be missing something. What has been changed, except for indentions?

@ddziaduch
Copy link
Contributor Author

@makasim at the end of the method there were three calls to end() method, now there are just two - the last was unnecessary

Copy link
Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

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

The change is correct.

We accidentally went and called Node::end() one additional time. Because there is no node above (which is what end method gives) it started failing once this method became strong-typed in Symfony.

In theory there is no need to call end() method at all, if we're not interested in traversing the tree. But it is done across all Symfony examples and is otherwise safe to do.

@p-pichet
Copy link
Contributor

Base on the explanation, it's ok for me, but can you add indentation please ?
I don't have right for merge it

@makasim
Copy link
Member

makasim commented Sep 24, 2024

got it, thanks!

@makasim makasim merged commit 3c1c778 into php-enqueue:master Sep 24, 2024
34 checks passed
@ddziaduch ddziaduch deleted the patch-1 branch September 24, 2024 07:58
@ddziaduch
Copy link
Contributor Author

@makasim can you please create a new tag? Thanks in advance 🙂

@ddziaduch
Copy link
Contributor Author

Is anybody able to help with this tag? @makasim @p-pichet @Steveb-p maybe?

@makasim
Copy link
Member

makasim commented Oct 1, 2024

https://github.com/php-enqueue/enqueue/releases/tag/0.10.23

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.

4 participants