KnpLabs / KnpMenuBundle

Object Oriented menus for your Symfony project.
http://knplabs.com
MIT License
1.4k stars 203 forks source link

Quick win - Fixing 3.4 deprecated warning #375

Closed stphane closed 3 years ago

stphane commented 6 years ago

Fix deprecation message introduced in Symfony 3.4

My attempt to push a new branch to issue a new PR was denied and so here might be the tiny change required.

--- a/src/Provider/BuilderAliasProvider.php
+++ b/src/Provider/BuilderAliasProvider.php
@@ -101,7 +101,7 @@ class BuilderAliasProvider implements MenuProviderInterface
             $logs = array();
             $bundles = array();

-            $allBundles = $this->kernel->getBundle($bundleName, false);
+            $allBundles = $this->kernel->getBundle($bundleName);

             // In Symfony 4, bundle inheritance is gone, so there is no way to get an array anymore.
stof commented 6 years ago

That's not a quick win, as it breaks support for bundle inheritance entirely (and people still using it in their project would then not be happy).

stof commented 6 years ago

And you cannot push branches directly to this repository. Only maintainers can (as for any project on github). See https://guides.github.com/activities/forking/ for the doc about it.

stphane commented 6 years ago

Thank you for your feedback. So, such warnings will remain in the dev bar until we update our project to symfony 4 and future coming releases of KnpMenuBundle ?

dhensen commented 6 years ago

@stphane if you update to symfony 4 now I believe that the KnpMenuBundle will be broken because of the deprecated 'false' in the getBundle call. A better fix with BC for 2.x and 3.x needs to be made or maybe a major update of KnpMenuBundle needs to be made to only support the new way of calling getBundle and drop the support for 2.x and 3.x in this new major version of KnPMenuBundlE

stof commented 6 years ago

no, it will not be broken with Symfony 4. The code already handles the case where an array is not returned anymore (and PHP itself does not forbid passing an extra argument)

stphane commented 6 years ago

@dhensen I do agree with this except the breaking part. (My question was, in other words, «Will there be an update to this bundle for 3.4.x to drop support for previous Symfony versions so that Warnings disappear»)

stof commented 6 years ago

@stphane Symfony 3.4 also supports bundle inheritance, even though it is deprecated. So removing this code entirely would be done only when dropping support for SF 3.4 itself too.

geelweb commented 6 years ago

As you already fixed the sf4 compatibility issue with

// In Symfony 4, bundle inheritance is gone, so there is no way to get an array anymore.
if (!is_array($allBundles)) {
    $allBundles = array($allBundles);
}

what about fix the deprecated with

$allBundles = $this->kernel->getBundle($bundleName, false, true);

this third parameter should remove the deprecated (https://github.com/symfony/symfony/blob/3.4/src/Symfony/Component/HttpKernel/Kernel.php)

DonCallisto commented 6 years ago

$allBundles = $this->kernel->getBundle($bundleName, false, true);

This won't work in Symfony4