search

sulu / SuluCommunityBundle

Community features like Login, Registration, Password forget/reset for your sulu application.
MIT License
30 stars 39 forks source link

Add compatibility with Sulu 2.5 - Symfony 6 #162

Closed dev-newvisibility closed 6 months ago

dev-newvisibility commented 1 year ago
Q A
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? yes
Related issues/PRs #159
License MIT

What's in this PR?

Compatibility with Sulu 2.5 - Symfony 6

BC Breaks/Deprecations

Removed compatibility with Swiftmailer (replaced with Symfony/Mailer) Removed compatibility with Symfony 4.4

Jupi007 commented 1 year ago

Hi, I'm testing this bundle with Sulu2.5 / Sf6.2 through you fork.

The only issue I can report for now is a problem when uploading an avatar. It throw an exception in the SaveMediaTrait:

Symfony\Component\ErrorHandler\Error\UndefinedMethodError:
Attempted to call an undefined method named "get" of class "Sulu\Bundle\CommunityBundle\Controller\ProfileController".
Did you mean to call e.g. "getSubscribedServices", "getSubscribedServicesOfSaveMediaTrait" or "getUser"?

  at vendor/sulu/community-bundle/Controller/SaveMediaTrait.php:112
  at Sulu\Bundle\CommunityBundle\Controller\ProfileController->getMediaManager()
     (vendor/sulu/community-bundle/Controller/SaveMediaTrait.php:87)
  at Sulu\Bundle\CommunityBundle\Controller\ProfileController->saveMedia()
     (vendor/sulu/community-bundle/Controller/SaveMediaTrait.php:78)
  at Sulu\Bundle\CommunityBundle\Controller\ProfileController->saveAvatar()
     (vendor/sulu/community-bundle/Controller/SaveMediaTrait.php:28)
  at Sulu\Bundle\CommunityBundle\Controller\ProfileController->saveMediaFields()
     (vendor/sulu/community-bundle/Controller/ProfileController.php:59)
  at Sulu\Bundle\CommunityBundle\Controller\ProfileController->indexAction()
     (vendor/symfony/http-kernel/HttpKernel.php:163)
  at Symfony\Component\HttpKernel\HttpKernel->handleRaw()
     (vendor/symfony/http-kernel/HttpKernel.php:74)
  at Symfony\Component\HttpKernel\HttpKernel->handle()
     (vendor/symfony/http-kernel/Kernel.php:184)
  at Symfony\Component\HttpKernel\Kernel->handle()
     (public/index.php:68)

Here:

https://github.com/sulu/SuluCommunityBundle/blob/0f96d4cc29813a3ee89fdaddf7948e978328a03c/Controller/SaveMediaTrait.php#L100-L111

instead of call $this->get() it should be $this->container->get().

Jupi007 commented 1 year ago

I just found another problem.

When a form isn't valid, an http 422 code need to be returned. It is required by Turbo, otherwise invalid form errors isn't handled correctly.

This is automatically done by the controller if you use renderForm() + $form, instead of render() + $form->createView(). https://symfony.com/doc/5.4/forms.html#rendering-forms

alexander-schranz commented 1 year ago

Sorry for the late response here.

First really thank you for the pull request. We are still Supporting Symfony 5.4 and SwiftMailer in this repository. Also the old symfony security System (PasswordEncoder). So the following things would be required to be tackled:

For the 422 status code I would create a seperate issue and PR as that should be out of scope of this update. /cc @Jupi007

Jupi007 commented 1 year ago

For the 422 status code I would create a seperate issue and PR as that should be out of scope of this update. /cc @Jupi007

@alexander-schranz Okay, makes sense. But as this feature is only available starting from Sf5.4, it will required to wait for this PR to be merged before fix it.

Edit: I created the issue #165

alexander-schranz commented 1 year ago

@Jupi007 i would not even use that method just make sure that the 422 is returned in case of errors.

Jupi007 commented 1 year ago

Oh, you're right. I hadn't thought at this possibility. In that case, this could be done at once.

MemoICT commented 1 year ago

Any news here ?

wachterjohannes commented 1 year ago

@alexander-schranz i have fixed a small issue with the completion listener! the rest of this branch works well. please review the code and merge it when its ok for you

alexander-schranz commented 12 months ago

The BC layers mention in https://github.com/sulu/SuluCommunityBundle/pull/162#issuecomment-1489918154 are still missing.

BC Layer for Mailer / Symfony Mailer like here: https://github.com/sulu/sulu/blob/7674bfcde288012d7bf321abe8259999a79ccda2/src/Sulu/Bundle/SecurityBundle/Controller/ResettingController.php#L448-L464

Zahiraa commented 11 months ago

is there a an available version compatible with Sulu 2.5 and symfony 6 ?

frickelbruder commented 8 months ago

Hi all,

I played around with the PR and stumbled upon some problems, if you really still want to maintain SwiftMailer compatibility. The requested BC-Layer code might be the smallest thing, as it is just copy and paste and surround with an if statement and replace the type hint with a union type. I could do that easily.

But when running the tests with swiftmailler installed, I got the following behaviour:

  1. If we use swiftmailer/swiftmailer as dependency, the sulu/sulu-TestBundle wants to include swiftmailer.yaml, which of course cannot handle bundle configs.
  2. So integrating symfony/swiftmailer-bundle helps. BUT this whole process downgrades the symfony kernel, which adds another odd situation: The Kernel from 6.4.5 to 5.4.37, which leads to a bug in the bc-layer in sulu core in File src/Sulu/Bundle/TestBundle/Resources/app/config/security-5-4.yml where the encoders node was renamed to password_hashers from Symfony 5.3. (By the way, the same bug is introduced in this PR)

So I am currently unsure, how you want to proceed here @alexander-schranz . To me it seems, this whole compatiblity layer does not work anyway. And I am unsure, as of how much work, I should put into this, with hopefully sulu 3 looming on the horizon anyway.

As I am in need of this bundle, I would be willing to put some effort in this.

alexander-schranz commented 6 months ago

Thx @dev-newvisibility @martinlagler