Skip to content

Add support of Symfony 3 and drop support for Symfony < 2.7 #289

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 7, 2016
Merged

Add support of Symfony 3 and drop support for Symfony < 2.7 #289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 33 additions & 18 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,45 @@
language: php

php:
- 5.3
- 5.4
- 5.5
- 5.6
- hhvm
sudo: false

env:
- SYMFONY_VERSION=2.1.*
- SYMFONY_VERSION=2.2.*
- SYMFONY_VERSION=2.3.*
- SYMFONY_VERSION=dev-master
branches:
only:
- master

matrix:
include:
- php: 5.3
env:
- deps=low
- SYMFONY_VERSION=2.7.*
Copy link
Member

@GuilhemN GuilhemN Sep 3, 2016

Choose a reason for hiding this comment

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

This looks useless and might hide forgotten dependencies, wdyt?

Copy link
Contributor Author

@tgalopin tgalopin Sep 4, 2016

Choose a reason for hiding this comment

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

Can you explain a bit more your idea? I don't understand why it would hide dependencies.

Copy link
Member

@GuilhemN GuilhemN Sep 4, 2016

Choose a reason for hiding this comment

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

Because you are always loading symfony entirely and you might depend on symfony libraries not included in the real composer file.

Copy link
Contributor Author

@tgalopin tgalopin Sep 4, 2016

Choose a reason for hiding this comment

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

We are requiring the framework bundle, not Symfony entirely, and we do require the framework bundle in the composer.json.

Copy link
Member

@GuilhemN GuilhemN Sep 4, 2016

Choose a reason for hiding this comment

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

Indeed sorry, i'm used to libraries loading the entire framework...
That's fine for me then 👍

- php: 5.4
env:
- deps=high
- SYMFONY_VERSION=2.7.*
- php: 5.5
env:
- deps=low
- SYMFONY_VERSION=2.8.*
- php: 5.6
env:
- deps=high
- SYMFONY_VERSION=3.0.*
- php: 7.0
env:
- deps=low
- SYMFONY_VERSION=3.1.*
- php: 7.1
env:
- deps=high
- SYMFONY_VERSION=3.1.*
Copy link
Member

@GuilhemN GuilhemN Sep 3, 2016

Choose a reason for hiding this comment

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

Do we actually need to test each version one by one? Imo as both php and symfony has a bc policy, we can only test the lowest and highest version of each major (php 5.3/sf 2.7/sf 3.0, php 7.1/sf 2./sf 3.).

Copy link
Contributor Author

@tgalopin tgalopin Sep 4, 2016

Choose a reason for hiding this comment

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

The PHP BC policy is quite messy and not really followed (see 7.1 BC breaks, and proposals for 7.2). I want to test every PHP version, I think it's more future-proof (and it does not add much overhead, it's done by Travis :) ). And if I test all the PHP versions, why not test the different Symfony versions as well? It's still a much lighter configuration than before, but test most of the cases.

Copy link
Member

@GuilhemN GuilhemN Sep 4, 2016

Choose a reason for hiding this comment

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

We know more or less which php versions are bc (so you could remove 5.4, 5.5 and 5.6 for example).
I'm not especially against testing the bundle in many environnements but it slows down the test build and it is not convenient for contributors imo.

Copy link
Contributor Author

@tgalopin tgalopin Sep 4, 2016

Choose a reason for hiding this comment

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

I agree with this argument, but for the moment the test suite run in less than 2 minutes. I think we should consider optimizing it if we need faster builds when we have a real test suite :) .


before_script:
- composer require symfony/framework-bundle:${SYMFONY_VERSION} --no-update
- composer update --dev
- if [[ $deps = high ]]; then composer update --no-progress --ansi; fi
- if [[ $deps = low ]]; then composer update --no-progress --ansi --prefer-lowest --prefer-stable; fi

script: phpunit --coverage-text

notifications:
email:
- friendsofsymfony-dev@googlegroups.com

matrix:
allow_failures:
- env: SYMFONY_VERSION=dev-master
- php: 5.6
- php: hhvm
28 changes: 21 additions & 7 deletions Controller/MessageController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,19 @@

namespace FOS\MessageBundle\Controller;

use Symfony\Component\DependencyInjection\ContainerAware;
use Symfony\Component\DependencyInjection\ContainerAwareInterface;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\HttpFoundation\RedirectResponse;
use FOS\MessageBundle\Provider\ProviderInterface;
use Symfony\Component\HttpFoundation\Response;

class MessageController extends ContainerAware
class MessageController implements ContainerAwareInterface
Copy link
Member

@GuilhemN GuilhemN Sep 3, 2016

Choose a reason for hiding this comment

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

This is the only bc break, should it be described in a UPGRADING file?

Copy link
Contributor Author

@tgalopin tgalopin Sep 4, 2016

Choose a reason for hiding this comment

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

Yes, I will do that.

{
/**
* @var ContainerInterface
*/
protected $container;

/**
* Displays the authenticated participant inbox
*
Expand Down Expand Up @@ -55,7 +61,7 @@ public function deletedAction()
* Displays a thread, also allows to reply to it
*
* @param string $threadId the thread id
*
*
* @return Response
*/
public function threadAction($threadId)
Expand Down Expand Up @@ -100,9 +106,9 @@ public function newThreadAction()

/**
* Deletes a thread
*
*
* @param string $threadId the thread id
*
*
* @return RedirectResponse
*/
public function deleteAction($threadId)
Expand All @@ -116,9 +122,9 @@ public function deleteAction($threadId)

/**
* Undeletes a thread
*
*
* @param string $threadId
*
*
* @return RedirectResponse
*/
public function undeleteAction($threadId)
Expand Down Expand Up @@ -155,4 +161,12 @@ protected function getProvider()
{
return $this->container->get('fos_message.provider');
}

/**
* {@inheritdoc}
*/
public function setContainer(ContainerInterface $container = null)
{
$this->container = $container;
}
}
4 changes: 2 additions & 2 deletions DependencyInjection/Configuration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public function getConfigTreeBuilder()
->addDefaultsIfNotSet()
->children()
->scalarNode('factory')->defaultValue('fos_message.new_thread_form.factory.default')->cannotBeEmpty()->end()
->scalarNode('type')->defaultValue('fos_message.new_thread_form.type.default')->cannotBeEmpty()->end()
->scalarNode('type')->defaultValue('FOS\MessageBundle\FormType\NewThreadMessageFormType')->cannotBeEmpty()->end()
->scalarNode('handler')->defaultValue('fos_message.new_thread_form.handler.default')->cannotBeEmpty()->end()
->scalarNode('name')->defaultValue('message')->cannotBeEmpty()->end()
->scalarNode('model')->defaultValue('FOS\MessageBundle\FormModel\NewThreadMessage')->end()
Expand All @@ -60,7 +60,7 @@ public function getConfigTreeBuilder()
->addDefaultsIfNotSet()
->children()
->scalarNode('factory')->defaultValue('fos_message.reply_form.factory.default')->cannotBeEmpty()->end()
->scalarNode('type')->defaultValue('fos_message.reply_form.type.default')->cannotBeEmpty()->end()
->scalarNode('type')->defaultValue('FOS\MessageBundle\FormType\ReplyMessageFormType')->cannotBeEmpty()->end()
->scalarNode('handler')->defaultValue('fos_message.reply_form.handler.default')->cannotBeEmpty()->end()
->scalarNode('name')->defaultValue('message')->cannotBeEmpty()->end()
->scalarNode('model')->defaultValue('FOS\MessageBundle\FormModel\ReplyMessage')->end()
Expand Down
30 changes: 28 additions & 2 deletions DependencyInjection/FOSMessageExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace FOS\MessageBundle\DependencyInjection;

use FOS\MessageBundle\Util\LegacyFormHelper;
use Symfony\Component\Config\Definition\Processor;
use Symfony\Component\DependencyInjection\Reference;
use Symfony\Component\HttpKernel\DependencyInjection\Extension;
Expand All @@ -23,6 +24,7 @@ public function load(array $configs, ContainerBuilder $container)
if (!in_array(strtolower($config['db_driver']), array('orm', 'mongodb'))) {
throw new \InvalidArgumentException(sprintf('Invalid db driver "%s".', $config['db_driver']));
}

$loader->load(sprintf('%s.xml', $config['db_driver']));
$loader->load('config.xml');
$loader->load('form.xml');
Expand Down Expand Up @@ -51,10 +53,34 @@ public function load(array $configs, ContainerBuilder $container)
$container->setAlias('fos_message.spam_detector', $config['spam_detector']);
$container->setAlias('fos_message.twig_extension', $config['twig_extension']);

$container->setAlias('fos_message.new_thread_form.type', $config['new_thread_form']['type']);
// BC management
$newThreadFormType = $config['new_thread_form']['type'];
$replyFormType = $config['reply_form']['type'];

if (!class_exists($newThreadFormType)) {
@trigger_error('Using a service reference in configuration key "fos_message.new_thread_form.type" is deprecated since version 1.2 and will be removed in 2.0. Use the class name of your form type instead.', E_USER_DEPRECATED);

// Old syntax (service reference)
$container->setAlias('fos_message.new_thread_form.type', $newThreadFormType);
} else {
// New syntax (class name)
$container->getDefinition('fos_message.new_thread_form.factory.default')
->replaceArgument(1, $newThreadFormType);
}

if (!class_exists($replyFormType)) {
Copy link
Member

@GuilhemN GuilhemN Sep 4, 2016

Choose a reason for hiding this comment

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

Can be merged with the check above thanks to a foreach

Copy link
Contributor Author

@tgalopin tgalopin Sep 4, 2016

Choose a reason for hiding this comment

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

I find this more readable than a foreach, as there would be a concatenated variable everywhere.

Copy link
Member

@GuilhemN GuilhemN Sep 5, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

no you could loop over ['new_thread_form', 'reply_form'] or `[$config['new_thread_form']['type'], $config['reply_form']['type']]`` if you prefer.

@trigger_error('Using a service reference in configuration key "fos_message.reply_form.type" is deprecated since version 1.2 and will be removed in 2.0. Use the class name of your form type instead.', E_USER_DEPRECATED);

// Old syntax (service reference)
$container->setAlias('fos_message.reply_form.type', $replyFormType);
} else {
// New syntax (class name)
$container->getDefinition('fos_message.reply_form.factory.default')
->replaceArgument(1, $replyFormType);
}

$container->setAlias('fos_message.new_thread_form.factory', $config['new_thread_form']['factory']);
$container->setAlias('fos_message.new_thread_form.handler', $config['new_thread_form']['handler']);
$container->setAlias('fos_message.reply_form.type', $config['reply_form']['type']);
$container->setAlias('fos_message.reply_form.factory', $config['reply_form']['factory']);
$container->setAlias('fos_message.reply_form.handler', $config['reply_form']['handler']);

Expand Down
6 changes: 3 additions & 3 deletions Document/Message.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ abstract class Message extends BaseMessage
* Tells if the message is spam or flood
* This denormalizes Thread.isSpam
*
* @var boolean
* @var bool
*/
protected $isSpam = false;

Expand All @@ -23,11 +23,11 @@ abstract class Message extends BaseMessage
protected $unreadForParticipants = array();

/**
* @param boolean $isSpam
* @param bool $isSpam
*/
public function setIsSpam($isSpam)
{
$this->isSpam = (boolean) $isSpam;
$this->isSpam = (bool) $isSpam;
}

/*
Expand Down
2 changes: 1 addition & 1 deletion Document/Thread.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ public function addParticipant(ParticipantInterface $participant)
* Tells if the user participates to the conversation
*
* @param ParticipantInterface $participant
* @return boolean
* @return bool
*/
public function isParticipant(ParticipantInterface $participant)
{
Expand Down
8 changes: 4 additions & 4 deletions DocumentManager/MessageManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ public function markAsUnreadByParticipant(ReadableInterface $readable, Participa
*
* @param ThreadInterface $thread
* @param ParticipantInterface $participant
* @param boolean $isRead
* @param bool $isRead
*/
public function markIsReadByThreadAndParticipant(ThreadInterface $thread, ParticipantInterface $participant, $isRead)
{
Expand All @@ -98,7 +98,7 @@ public function markIsReadByThreadAndParticipant(ThreadInterface $thread, Partic
*
* @param MessageInterface $message
* @param ParticipantInterface $participant
* @param boolean $isRead
* @param bool $isRead
*/
protected function markIsReadByParticipant(MessageInterface $message, ParticipantInterface $participant, $isRead)
{
Expand All @@ -112,7 +112,7 @@ protected function markIsReadByParticipant(MessageInterface $message, Participan
* by updating directly the storage
*
* @param ParticipantInterface $participant
* @param boolean $isRead
* @param bool $isRead
* @param \Closure $condition
*/
protected function markIsReadByCondition(ParticipantInterface $participant, $isRead, \Closure $condition)
Expand All @@ -132,7 +132,7 @@ protected function markIsReadByCondition(ParticipantInterface $participant, $isR
}

$queryBuilder
->field('metadata.$.isRead')->set((boolean) $isRead)
->field('metadata.$.isRead')->set((bool) $isRead)
->getQuery(array('multiple' => true))
->execute();

Expand Down
4 changes: 2 additions & 2 deletions EntityManager/MessageManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public function markAsUnreadByParticipant(ReadableInterface $readable, Participa
*
* @param ThreadInterface $thread
* @param ParticipantInterface $participant
* @param boolean $isRead
* @param bool $isRead
*/
public function markIsReadByThreadAndParticipant(ThreadInterface $thread, ParticipantInterface $participant, $isRead)
{
Expand All @@ -111,7 +111,7 @@ public function markIsReadByThreadAndParticipant(ThreadInterface $thread, Partic
*
* @param MessageInterface $message
* @param ParticipantInterface $participant
* @param boolean $isRead
* @param bool $isRead
*/
protected function markIsReadByParticipant(MessageInterface $message, ParticipantInterface $participant, $isRead)
{
Expand Down
16 changes: 13 additions & 3 deletions FormFactory/AbstractMessageFormFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ abstract class AbstractMessageFormFactory
/**
* The message form type
*
* @var AbstractType
* @var AbstractType|string
*/
protected $formType;

Expand All @@ -41,8 +41,16 @@ abstract class AbstractMessageFormFactory
*/
protected $messageClass;

public function __construct(FormFactoryInterface $formFactory, AbstractType $formType, $formName, $messageClass)
public function __construct(FormFactoryInterface $formFactory, $formType, $formName, $messageClass)
{
if (!is_string($formType) && !$formType instanceof AbstractType) {
throw new \InvalidArgumentException(sprintf(
'Form type provided is not valid (class name or instance of %s expected, %s given)',
'Symfony\Component\Form\AbstractType',
is_object($formType) ? get_class($formType) : gettype($formType)
));
}

$this->formFactory = $formFactory;
$this->formType = $formType;
$this->formName = $formName;
Expand All @@ -56,6 +64,8 @@ public function __construct(FormFactoryInterface $formFactory, AbstractType $for
*/
protected function createModelInstance()
{
return new $this->messageClass();
$class = $this->messageClass;

return new $class();
}
}
Loading