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.

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

Jump to bottom

Upgrade to support 2.8 & 3.0 and fixed deprecations #80

Merged
merged 3 commits into from
Mar 14, 2016
Merged

Upgrade to support 2.8 & 3.0 and fixed deprecations #80

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
526b4c8
Upgrade to support 2.8 & 3.0 and fixed deprecations
trq Dec 2, 2015
f059e75
Updated composer.json re PR
trq Dec 7, 2015
a3facf3
Updated .travis.yml re PR
trq Dec 7, 2015
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
6 changes: 6 additions & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,18 @@ php:
- 5.3
- 5.4
- 5.5
- 5.6

env:
- SYMFONY_VERSION=2.1.*
- SYMFONY_VERSION=2.2.*
- SYMFONY_VERSION=2.3.*
- SYMFONY_VERSION=2.4.*
- SYMFONY_VERSION=2.5.*
- SYMFONY_VERSION=2.6.*
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't add tests for these two versions as they already reached their end of maintenance (2.5 actually even reached its end of life). For the same reason you might want to drop the build for 2.4 too (the builds for 2.1 and 2.2 actually do not make much sense at all as the MobileDetectBundle already requires the version of the FrameworkBundle to be 2.3 or higher).

- SYMFONY_VERSION=2.7.*
- SYMFONY_VERSION=2.8.*
- SYMFONY_VERSION=3.0.*

before_script:
- composer require symfony/framework-bundle:${SYMFONY_VERSION}
Expand Down
4 changes: 1 addition & 3 deletions Helper/DeviceView.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,14 +54,12 @@ class DeviceView
*/
public function __construct(Container $serviceContainer)
{
if (false === $serviceContainer->isScopeActive('request')) {
if (!$this->request = $serviceContainer->get('request_stack')->getMasterRequest()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work in Symfony 2.3 or older as the request stack was introduced in Symfony 2.4.

Copy link
Contributor

Choose a reason for hiding this comment

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

A solution might be to register a request event listener which populates the current request to the DeviceView by calling a method like setRequest() (to be introduced).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm wondering if it might be better / easier to break BC and bump the major version?

On 2 Dec 2015, at 19:40, Christian Flothmann notifications@github.com wrote:

In Helper/DeviceView.php:

@@ -54,14 +54,12 @@ class DeviceView
*/
public function __construct(Container $serviceContainer)
{

  •    if (false === $serviceContainer->isScopeActive('request')) {

  •    if (!$this->request = $serviceContainer->get('request_stack')->getMasterRequest()) {
    This won't work in Symfony 2.3 or older as the request stack was introduced in Symfony 2.4.

—
Reply to this email directly or view it on GitHub.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, Symfony 2.3 is still supported until May and will then receive security fixes for another year. So personally, I would not make that change yet.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Bumping the major version doesn't mean the (then) older major won't work on Symfony 2.3 anymore. If need be, we could even create a branch and release a patch on the (then) older major.

I've seen a couple of bundles taking this approach to avoid the boilerplate code required to support both sf <= 2.3 and sf >= 2.4 at the same time. Plus, once sf 2.3 goes end-of-life, we'd have to go through our codebase (again) and explicitly remove the boilerplate code, which takes time (again).
So personally, I like the idea of bumping the major version and clearly documenting the intention behind it in the docs and the composer.json's dependency declarations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well put, a and is precisely why I made the suggestion.

On 9 Feb 2016, at 23:12, netmikey notifications@github.com wrote:

In Helper/DeviceView.php:

@@ -54,14 +54,12 @@ class DeviceView
*/
public function __construct(Container $serviceContainer)
{

  •    if (false === $serviceContainer->isScopeActive('request')) {

  •    if (!$this->request = $serviceContainer->get('request_stack')->getMasterRequest()) {
    Bumping the major version doesn't mean the (then) older major won't work on Symfony 2.3 anymore. If need be, we could even create a branch and release a patch on the (then) older major.
    I've seen a couple of bundles taking this approach to avoid the boilerplate code required to support both sf <= 2.3 and sf >= 2.4 at the same time. Plus, once sf 2.3 goes end-of-life, we'd have to go through our codebase (again) and explicitly remove the boilerplate code, which takes time (again).
    So personally, I like the idea of bumping the major version and clearly documenting the intention behind it in the docs and the composer.json's dependency declarations.

—
Reply to this email directly or view it on GitHub.

$this->viewType = self::VIEW_NOT_MOBILE;

return;
}

$this->request = $serviceContainer->get('request');

if ($this->request->query->has(self::SWITCH_PARAM)) {
$this->viewType = $this->request->query->get(self::SWITCH_PARAM);
} elseif ($this->request->cookies->has(self::COOKIE_KEY)) {
Expand Down
5 changes: 3 additions & 2 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,12 @@
],
"require": {
"php": ">=5.3.0",
"symfony/framework-bundle": ">=2.3,<3.0",
"symfony/framework-bundle": ">=2.3|~2.8|~3.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

you can simply use ~2.3|~3.0

"mobiledetect/mobiledetectlib": "~2.8"
},
"require-dev": {
"phpunit/phpunit": "~4.1"
"phpunit/phpunit": "~4.1",
"symfony/phpunit-bridge": "^3.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be ^2.7|3.0 (3.x requires PHP 5.5.9 or higher).

},
"autoload": {
"psr-0": {
Expand Down