-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Adding Symfony4 support #2639
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
Conversation
.travis.yml
Outdated
@@ -14,6 +14,8 @@ env: | |||
matrix: | |||
fast_finish: true | |||
include: | |||
- php: 7.1 | |||
env: DEPENDENCIES=beta |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :)
Does this include support for symfony/flex, seeing as that will be the default method of project creation in v4.0? |
@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; |
There was a problem hiding this comment.
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?
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... |
@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. |
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 |
Perhaps it is better to create a 2.x branch for bug fixes, and drop Symfony 2.x support in master. |
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. |
I've just created 2.0.x branch and bumped master to 3.0.x-dev
|
@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). 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. |
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. |
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). |
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" |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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! |
+1 |
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
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.
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.
Yes I bumped master back to 2.1
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; |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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)
@weaverryan please rebase, as this PR conflicts with #2651 dropping SF 2.7 support to make maintenance easier |
Ok! This should now be ready:
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). |
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. |
@asmaHemden |
I see people mentioning fos_user.yaml - is this meant to be created? After I can create it myself, just though ti might need to be raised.
|
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 |
Any idea when a stable 2.1 release comes out? |
@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 First off, I followed the example here exactly, only I put my form in a bundle and used the field
That's really the main difference.
Now I did this and got your getter/setter issue. Realized I hadn't added
into my |
@richard4339 thanks a lot, I'll try this configuration. |
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! |
Hi @aleixfabra,
we use composer: "friendsofsymfony/user-bundle": "^2.1" |
It will be Symfony 4.1 support soon? Thanks! |
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. |
@aleixfabra
@Tomsgu do you use the dev-master? |
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 |
Thanks for not updating the docs for SF4 👎 |
@RoestVrijStaal This is open source. You could be the one contributing the documentation update. |
@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?). |
@RoestVrijStaal I don't want to read passive-aggressive, selfish comments on open source projects, and yet here we are. |
@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 :-) |
@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 ;) |
In docs there is written:
But in symfony 4 there is not catalog Know anyone how to override FOS views in symfony4? |
Ok. I found instead
|
Hi @gustawdaniel , views can be overridden in templates/bundles/FOSUserBundle/... we use symfony 4.1 and it works without any problem |
Thanks @yonisolo but in my case Ok I had installed version
When I updated to There is no error:
And move templates to |
yes if you have only views to override, you don't need to add /src/Resources/FOSUserBundle |
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!