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

Adding Symfony4 support #2639

Merged
merged 27 commits into from Dec 18, 2017
Merged

Adding Symfony4 support #2639

merged 27 commits into from Dec 18, 2017

Conversation

weaverryan
Copy link

Hi guys!

I bumped the deps, made Travis test it, and fixed one test. Unless something isn't covered... this seems like an easy win!

Btw, there seems to be little consistency across bundles about how to organize .travis.yml so that Symfony 4 beta is included. I thought adding a "beta" stability test to the matrix was a good idea: it should always test the latest stable, or beta release I believe. But if I'm wrong of there's a better way, I'd love to know.

Cheers!

.travis.yml Outdated
@@ -14,6 +14,8 @@ env:
matrix:
fast_finish: true
include:
- php: 7.1
env: DEPENDENCIES=beta
Copy link
Member

Choose a reason for hiding this comment

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

What about also adding an explicit job for 3.4 to have a dedicated job for the next LTS release?

Copy link
Author

Choose a reason for hiding this comment

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

That's a good idea: I've re-ordered and restructured things. I think it makes sense to test each LTS release, and then also the latest release. Hopefully I've just done that :)

@xabbuh xabbuh mentioned this pull request Nov 10, 2017
@tip2tail
Copy link

Does this include support for symfony/flex, seeing as that will be the default method of project creation in v4.0?

@weaverryan
Copy link
Author

@tip2tail There's nothing special about Flex that would need to be supported, except for the fact that the Flex directory structure is bundle-less, and I don't think FOSUserBundle relies on your app having a bundle.

So, yes :). Unrelated to this PR, it would be great to add a recipe for FOSUserBundle.

.travis.yml Outdated
@@ -32,6 +40,7 @@ cache:
- $HOME/.composer/cache

before_install:
- if [ "$DEPENDENCIES" = "beta" ]; then perl -pi -e 's/^}$/,"minimum-stability":"beta"}/' composer.json; fi;
Copy link

Choose a reason for hiding this comment

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

Maybe a composer config minimum-stability beta command is better?

@Jean85
Copy link

Jean85 commented Nov 15, 2017

Also, quick generic question: isn't dangerous allowing 3 different major versions of Symfony in the same bundle? To me, it seems an easy way to have issues of cross-compat down the line...

@weaverryan
Copy link
Author

@Jean85 There's no official best-practice about this that I'm aware of. Should bundles support all supported Symfony versions? Or should they only support the last few versions of Symfony when releasing minor versions (with new features)? The safest bet is to support all supported versions. I don't think there's anything dangerous about this - but it is potentially more work for bundle maintainers to keep their code compatible across so many versions.

@Jean85
Copy link

Jean85 commented Nov 16, 2017

Yep, the more work is what I'm worried about. As an example, something deprecated in 3.x will be missing in 4 but will have an alternative, but only on 3, not on 2!

In my case (I'm maintaining sentry/sentry-symfony) I chose to have 2 major versions, one with ^2.7||^3.0, the other (to be released soon) with ^3.0||^4.0; this IMHO has the best balance between easier maintainability and better user experience, especially when upgrading between SF versions.

@XWB
Copy link
Member

XWB commented Nov 16, 2017

Perhaps it is better to create a 2.x branch for bug fixes, and drop Symfony 2.x support in master.

@weaverryan
Copy link
Author

I actually like the idea of supporting 2 major versions only. It means that as soon as 4.0 is released, there is a new LTS (3.4) and so users will need to upgrade from the old LTS to the new LTS in order to get the new features from the bundle. I think that's a fair requirement.

Then, technically, bundles should continue to offer bug fixes for Symfony versions until they hit end-of-life. But, that's extra work. So maybe some bundles could do that, but I wouldn't expect all community bundles to do that in reality.

So I'm +1 for only supporting 2 major versions. And in well-maintained / popular bundles, we should aim to also make bug fixes available to all supported Symfony versions. And that should probably be up to the maintainer to decide.

@XWB
Copy link
Member

XWB commented Nov 20, 2017

I've just created 2.0.x branch and bumped master to 3.0.x-dev

  • We should deprecate the 1.3.x branch
  • We should keep the 2.0.x branch for bug fixes
  • We should use master (3.0.x) for BC breaks.

@stof
Copy link
Member

stof commented Nov 21, 2017

@XWB please don't make us maintain 2.x and 3.x of the bundle yet. Dropping support of old Symfony versions is not a BC break (it is one only for people not using Composer, as Composer covers it for them, but adding a dependency would also be a BC break for these people so I don't care about them).
New features of FOSUserBundle should be added in a minor version. So master should be 2.1.x, not 3.0.x.

And btw, I don't think dropping support for 2.8 would simplify the maintenance at all (dropping 2.7 will simplify it due to the Form BC layer). If it is easy to support 2.8 to 4.0 in the codebase, I'd rather do it than maintaining 2 branches in parallel to make bug fixes available to 2.8 users (maintaining a single branch is less work for us.

@Jean85
Copy link

Jean85 commented Nov 22, 2017

I agree with @stof but I stand by my opinion, supporting max 2 major of Symfony for each major of a bundle should be the limit; 2.8 is just a special case because it's the same as supporting 3.0, so it has the same BC promise as the 3.x SF major.

@xabbuh
Copy link
Member

xabbuh commented Nov 22, 2017

2.8 is just a special case because it's the same as supporting 3.0, so it has the same BC promise as the 3.x SF major.

This will always be the case when new major versions are released (e.g. when 5.0 is released, 3.4 is still maintained by the core team with the same feature set as 4.0).

@weaverryan
Copy link
Author

Regardless of what we decide to support, unless I'm mistaken, this should be merged into master and then we need a new tag. This is the most popular Symfony bundle - we really need a compatible version. We can decide later exactly how old of versions we want to support.

@XWB could you merge and prepare a tab? 😇

composer.json Outdated
"symfony/console": "^2.7 || ^3.0 || ^4.0",
"symfony/phpunit-bridge": "^2.7 || ^3.0 || ^4.0",
"symfony/validator": "^2.7 || ^3.0 || ^4.0",
"symfony/yaml": "^2.7 || ^3.0 || ^4.0",
"phpunit/phpunit": "~4.8.35|~5.4.3"
Copy link
Member

Choose a reason for hiding this comment

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

Do you really want to exclude 5.5+? The constraint should probably be ^4.8.35|^5.4.3 (for 4.x it actually doesn't really matter as there is no 4.9 release).

Choose a reason for hiding this comment

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

Probably not relevant already? Looks like Ryan just reverted it to the original line

Copy link

Choose a reason for hiding this comment

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

It's surely not relevant to this PR. We can address that elsewhere.

Copy link

Choose a reason for hiding this comment

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

I've proposed the fix in #2647

@weaverryan
Copy link
Author

Ping @XWB! Could you merge this and create a release? Symfony 4 comes Thu, and this patch is quote tiny :). This is the 1st or 2nd most popular bundle that is still not Symfony 4 compat. I hope we can fix that!

@l3ackslash0
Copy link

+1

@weaverryan
Copy link
Author

Conflict fixed! @XWB ... a merge and a tag? I'd love to have this be ready before Symfony 4 tomorrow...

.travis.yml Outdated
- php: 5.6
env: SYMFONY_VERSION=2.8.*
# test the latest stable 3.x release
- php: 5.6
env: SYMFONY_VERSION=^3.0
Copy link
Member

@stof stof Nov 29, 2017

Choose a reason for hiding this comment

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

this does not make any sense. It should test the LTS. We don't want to have jobs for each minor

Copy link

Choose a reason for hiding this comment

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

Well, it's ^3 so since tomorrow the latest LTS and the version for this job will be the same!

Copy link
Member

Choose a reason for hiding this comment

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

But this will use 3.3 which is not an LTS release.

.travis.yml Outdated
env: SYMFONY_VERSION=^3.0
# test the latest release (including beta releases)
- php: 7.1
env: DEPENDENCIES=beta
Copy link
Member

Choose a reason for hiding this comment

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

Please use a DEPENDENCIES=dev instead of beta. It would make the job much more useful.

@xabbuh xabbuh mentioned this pull request Nov 29, 2017
@XWB
Copy link
Member

XWB commented Nov 29, 2017

PR looks good in general, though I would like to see the beta dependency removed. We should test against the latest stable Symfony 4.x release.

Should bundles support all supported Symfony versions?

It kind of bothers me that a new major Symfony version can have all BC layers removed, yet bundles need to keep those layers. It feels counter intuitive.

Please don't make us maintain 2.x and 3.x of the bundle yet. Dropping support of old Symfony versions is not a BC break

Yes I bumped master back to 2.1

And btw, I don't think dropping support for 2.8 would simplify the maintenance at all (dropping 2.7 will simplify it due to the Form BC layer). If it is easy to support 2.8 to 4.0 in the codebase

Thanks, I'm gonna drop Symfony 2.7 support.

.travis.yml Outdated
@@ -32,6 +36,7 @@ cache:
- $HOME/.composer/cache

before_install:
- if [[ -v $STABILITY ]]; then composer config minimum-stability $STABILITY; fi;
- if [ "$SYMFONY_VERSION" != "" ]; then composer require "symfony/symfony:${SYMFONY_VERSION}" --no-update; fi;
Copy link
Member

Choose a reason for hiding this comment

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

this line must be updated, as you replaced SYMFONY_VERSION=2.8.* by DEPENDENCIES="symfony/lts:^2" in the job matrix and this is the corresponding one

.travis.yml Outdated
allow_failures:
- php: 7.1
Copy link
Member

Choose a reason for hiding this comment

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

Remove php: 7.1 from the conditions matching the allowed failure (matching any PHP version as long as it has env: STABILITY="dev"). This way, we have a single place to update when changing the PHP version for the dev job

.travis.yml Outdated
allow_failures:
- php: 7.1
env: STABILITY="dev"
- php: hhvm
Copy link
Member

Choose a reason for hiding this comment

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

I suggest removing testing on HHVM as HHVM stopped bothering about PHP compat and Symfony dropped support for it. I see no interest into debugging HHVM failures if they don't care about PHP compat, so I don't care about having it on CI (I removed it from any project I maintain for which I reworked the Travis config after their announcement)

@stof
Copy link
Member

stof commented Nov 29, 2017

@weaverryan please rebase, as this PR conflicts with #2651 dropping SF 2.7 support to make maintenance easier

@weaverryan
Copy link
Author

Ok! This should now be ready:

  1. I made several changes to .travis.yml from Stof's feedback
  2. I added public=true to all currently-public services and aliases (except for listeners & form types - these should be public, and by not adding making any changes, they will become private in 4.0).

I think we should merge this immediately. That will make it easier to test. Then, we can tag a new version shortly after (probably people from the community can help us test, once it's merged).

@JVerstry
Copy link

About the command issue, I have opened a question on SO a couple of days ago: https://stackoverflow.com/questions/48465189/error-there-are-no-commands-defined-in-the-fosuser-namespace Glad to see it's on the radar here.

@MounaHelloPi
Copy link

MounaHelloPi commented Jan 29, 2018

@asmaHemden
you added the code line under config/routes.yaml
when you should add a file under config/routes called "fos_user.yaml"
than add the code below :
fos_user: resource: "@FOSUserBundle/Resources/config/routing/all.xml"

@MartinLyne
Copy link

I see people mentioning fos_user.yaml - is this meant to be created?

After composer require friendsofsymfony/user-bundle "dev-master" I get The child node "db_driver" at path "fos_user" must be configured.

I can create it myself, just though ti might need to be raised.

ll config/packages/
total 56
drwxrwxr-x 5 vagrant vagrant 4096 Feb  5 13:19 ./
drwxrwxr-x 4 vagrant vagrant 4096 Feb  5 13:17 ../
drwxrwxr-x 2 vagrant vagrant 4096 Feb  5 13:08 dev/
-rw-rw-r-- 1 vagrant vagrant  233 Feb  5 13:08 doctrine_migrations.yaml
-rw-rw-r-- 1 vagrant vagrant  894 Feb  5 13:08 doctrine.yaml
-rw-rw-r-- 1 vagrant vagrant  935 Feb  5 13:08 framework.yaml
drwxrwxr-x 2 vagrant vagrant 4096 Feb  5 13:08 prod/
-rw-rw-r-- 1 vagrant vagrant   54 Feb  5 13:08 routing.yaml
-rw-rw-r-- 1 vagrant vagrant  598 Feb  5 13:08 security.yaml
-rw-rw-r-- 1 vagrant vagrant   72 Feb  5 13:08 swiftmailer.yaml
drwxrwxr-x 2 vagrant vagrant 4096 Feb  5 13:08 test/
-rw-rw-r-- 1 vagrant vagrant  167 Feb  5 13:08 translation.yaml
-rw-rw-r-- 1 vagrant vagrant  252 Feb  5 13:08 twig_extensions.yaml
-rw-rw-r-- 1 vagrant vagrant  119 Feb  5 13:08 twig.yaml

@blackbart420
Copy link

Hello @MartinLyne

with SF4, the bundle should create the config file fos_user.yaml but for now you have to add it by yourself.

You can have a look at the content of my files fos_user.yaml and security.yaml up there

@ceesvanegmond
Copy link

Any idea when a stable 2.1 release comes out?

@mirkorap
Copy link

How can I override the RegistrationFormType in Symfony 4? I do in this way, but it doesn't work:

My RegistrationType
image

Explicit service config (services.yaml)
image

fos_user.yaml:
image

In my entity User class, I declare firstname as protected property and setter/getter method of it (setFirstname and getFirstname). Of course my entity extends BaseUser of FOSUserBundle.
image

But in my Twig template, if I use form_widget(form.firstname), I get an error:

Twig template:
image

The error is:

Neither the property "firstname" nor one of the methods "firstname()", "getfirstname()"/"isfirstname()"/"hasfirstname()" or "__call()" exist and have public access in class "Symfony\Component\Form\FormView".

@richard4339
Copy link

@mirkorap it took me a bit to get this, but using the latest documentation I finally got it!

What I can't see from your example above is if you did add the getter/setters, and tbh since you got that far I'm assuming your RegistrationType class and service registration is fine. However, I'm putting everything I did below.

First off, I followed the example here exactly, only I put my form in a bundle and used the field timezone.

<?php

namespace App\CWM\CWMBundle\Form;

use Symfony\Component\Form\AbstractType;
use Symfony\Component\Form\FormBuilderInterface;

class RegistrationType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $builder
            ->add('timezone')
        ;
    }

    public function getParent()
    {
        return 'FOS\UserBundle\Form\Type\RegistrationFormType';
    }

    public function getBlockPrefix()
    {
        return 'app_user_registration';
    }
}

That's really the main difference.

fos_user:
    registration:
        form:
            type: App\CWM\CWMBundle\Form\RegistrationType`

`services:
    app.form.registration:
        class: App\CWM\CWMBundle\Form\RegistrationType
        tags:
            - { name: form.type, alias: app_user_registration  }

Now I did this and got your getter/setter issue. Realized I hadn't added

    /**
     * @return \DateTimeZone
     */
    public function getTimezone()
    {
        return $this->timezone;
    }

    /**
     * @param \DateTimeZone $timezone
     * @return User
     */
    public function setTimezone(\DateTimeZone $timezone): User
    {
        $this->timezone = $timezone;
        return $this;
    }

into my User class. Once I did this it worked.

image

@mirkorap
Copy link

@richard4339 thanks a lot, I'll try this configuration.

@aleixfabra
Copy link

Hi guys!

How is FOSUserBundle Symfony 4 support? Is it fully integrated?

For example, @blackbart420 said that "with SF4, the bundle should create the config file fos_user.yaml but for now you have to add it by yourself."

It also seems that the documentation is not updated (it uses files paths from Symfony 3) https://symfony.com/doc/master/bundles/FOSUserBundle/index.html

Is there any guide for Symfony 4 setup?

I would be grateful if anyone could fill me in with the current state of the integration.

Thanks!

@yonisolo
Copy link

yonisolo commented Jul 19, 2018

Hi @aleixfabra,
we use it with symfony 4.0.12 without any problem, however there is still lots of deprecated features avoiding us to use it with symfony 4.1, for example User Checker and advanced User Interface...
But this still works fine. Since we did the migration some time ago, I don't know if the config file is generated or not. here an example of config file which is working for us:

fos_user:
    db_driver: orm
        firewall_name: main
    user_class: App\Entity\User
    group:
        group_class: App\Entity\Group
    from_email:
        address: "donotreply@test.com"
        sender_name: "Account Test"
    use_flash_notifications: false
    service:
        mailer: fos_user.mailer.twig_swift
    resetting:
        token_ttl: 86400
        email:
            from_email:
                address:        donotreply@test.com
                sender_name:    "Account Test"
            template:   email/password_resetting.email.twig
    registration:
        confirmation:
            enabled: true
            from_email:
                address:        donotreply@test.com
                sender_name:    "Account creation"
            template: email/confirm_registration.email.twig
        form:
            type: App\Form\RegistrationFormType

we use composer: "friendsofsymfony/user-bundle": "^2.1"

@aleixfabra
Copy link

It will be Symfony 4.1 support soon?

Thanks!

@Tomsgu
Copy link

Tomsgu commented Jul 20, 2018

I am using it with Symfony 4.1 in more than 3 different projects without any problems. So I don’t understand what you’re asking about.

@yonisolo
Copy link

@aleixfabra
Obviously, there are just few deprecated features for 4.1 but this should work, we just avoid it for the moment:

The "FOS\UserBundle\Model\UserInterface" interface extends "Symfony\Component\Security\Core\User\AdvancedUserInterface" that is deprecated since Symfony 4.1 *.

@Tomsgu do you use the dev-master?

@aleixfabra
Copy link

aleixfabra commented Jul 23, 2018

This has worked for me:

$ composer require friendsofsymfony/user-bundle "^2.1"
// config/packages/fos_user.yaml

fos_user:
    db_driver: orm
    firewall_name: main
    user_class: App\Entity\User
    from_email:
        address: test
        sender_name: test
// config/packages/framework.yaml

framework:
    // ...

    templating:
        engines: ['twig']
// config/packages/security.yaml

security:
    encoders:
        FOS\UserBundle\Model\UserInterface: bcrypt

    role_hierarchy:
        ROLE_ADMIN:       ROLE_USER
        ROLE_SUPER_ADMIN: ROLE_ADMIN

    # https://symfony.com/doc/current/security.html#where-do-users-come-from-user-providers
    providers:
        in_memory: { memory: ~ }
        fos_userbundle:
            id: fos_user.user_provider.username

    firewalls:
        dev:
            pattern: ^/(_(profiler|wdt)|css|images|js)/
            security: false
        main:
            pattern: ^/
            form_login:
                provider: fos_userbundle
                csrf_token_generator: security.csrf.token_manager
            logout:       true
            anonymous:    true

    # Easy way to control access for large sections of your site
    # Note: Only the *first* access control that matches will be used
    access_control:
        - { path: ^/login$, role: IS_AUTHENTICATED_ANONYMOUSLY }
        - { path: ^/register, role: IS_AUTHENTICATED_ANONYMOUSLY }
        - { path: ^/resetting, role: IS_AUTHENTICATED_ANONYMOUSLY }
        - { path: ^/admin/, role: ROLE_ADMIN }
// config/routes/fos_user.yaml

fos_user:
    resource: "@FOSUserBundle/Resources/config/routing/all.xml"
<?php

namespace App\Entity;

use Doctrine\ORM\Mapping as ORM;
use FOS\UserBundle\Model\User as BaseUser;

/**
 * @ORM\Entity(repositoryClass="App\Repository\UserRepository")
 */
class User extends BaseUser
{
    /**
     * @ORM\Id()
     * @ORM\GeneratedValue()
     * @ORM\Column(type="integer")
     */
    protected $id;

    public function __construct()
    {
        parent::__construct();
    }
}
<?php

namespace App\Repository;

use App\Entity\User;
use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository;
use Symfony\Bridge\Doctrine\RegistryInterface;

/**
 * @method User|null find($id, $lockMode = null, $lockVersion = null)
 * @method User|null findOneBy(array $criteria, array $orderBy = null)
 * @method User[]    findAll()
 * @method User[]    findBy(array $criteria, array $orderBy = null, $limit = null, $offset = null)
 */
class UserRepository extends ServiceEntityRepository
{
    public function __construct(RegistryInterface $registry)
    {
        parent::__construct($registry, User::class);
    }
}
$ php bin/console doctrine:schema:update --force

@RoestVrijStaal
Copy link

Thanks for not updating the docs for SF4 👎

@xabbuh
Copy link
Member

xabbuh commented Aug 1, 2018

@RoestVrijStaal This is open source. You could be the one contributing the documentation update.

@RoestVrijStaal
Copy link

RoestVrijStaal commented Aug 2, 2018

@xabbuh Yes, i know.

BUT. I don't know anything about the internals (read: the internals) of FOSUserBundle nor Symfony, nor I want to know those (Blackboxing, anyone?).

@danabrey
Copy link

danabrey commented Aug 2, 2018

@RoestVrijStaal I don't want to read passive-aggressive, selfish comments on open source projects, and yet here we are.

@ceesvanegmond
Copy link

@danabrey Totally agree with you. @RoestVrijStaal Welcome to open source. If you want to add new documentation, please do! We're all in this together :-)

@polishExperiment
Copy link

@xabbuh I would like to contribute to documentation update. I don't know how though. Can You or someone point me towards how to do this?

@CyrilCharlier
Copy link

@xabbuh I would like to contribute to documentation update. I don't know how though. Can You or someone point me towards how to do this?

Just go to this adress :

https://github.com/FriendsOfSymfony/FOSUserBundle/edit/master/Resources/doc/index.rst

;)

@gustawdaniel
Copy link

gustawdaniel commented Dec 10, 2018

In docs there is written:

The easiest way to override a bundle's template is to simply place a new one in your app/Resources folder. To override the layout template located at Resources/views/layout.html.twig in the FOSUserBundle directory, you would place your new layout template at app/Resources/FOSUserBundle/views/layout.html.twig.

But in symfony 4 there is not catalog app and twig files are located in /templates by default.

Know anyone how to override FOS views in symfony4?

@gustawdaniel
Copy link

Ok. I found instead app I should use src

/src/Resources/FOSUserBundle

@yonisolo
Copy link

yonisolo commented Dec 10, 2018

Hi @gustawdaniel , views can be overridden in templates/bundles/FOSUserBundle/... we use symfony 4.1 and it works without any problem

@gustawdaniel
Copy link

Thanks @yonisolo but in my case templates/bundles/FOSUserBundle/views/... does not work...

Ok I had installed version "friendsofsymfony/user-bundle": "~2.0" because of I followed by tutorial:

https://ourcodeworld.com/articles/read/794/how-to-install-and-configure-fosuserbundle-in-symfony-4

When I updated to 2.1

There is no error:

Executing script cache:clear [KO]
 [KO]
Script cache:clear returned with error code 1
!!  
!!  In ArrayNode.php line 228:
!!                                                                       
!!    The child node "db_driver" at path "fos_user" must be configured.  
!!                                                                       
!!  
!!  
Script @auto-scripts was called via post-update-cmd

And move templates to templates/bundles/FOSUserBundle/ works only if you remove directory views

zrzut ekranu z 2018-12-10 11-20-55

@yonisolo
Copy link

yes if you have only views to override, you don't need to add /src/Resources/FOSUserBundle
directory and all logic behind, but if you need to override more logic, then I don't know if it's better to separate view templates from src or keep it inside...

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.

None yet