Drop old versions #221

greg0ire · 2016-11-26T16:03:57Z

I am targetting this branch, because bumping minor version of some deps is considered major.

Changelog

### Removed
- support for PHP 5.3 through 5.5 was dropped
- support for Symfony 2.3 through 2.7 was dropped

Todo

merge Add missing deprecations #222
merge 3.x into master
add upgrade note
rebase

Subject

This prepares the next major release

greg0ire · 2016-11-28T16:34:07Z

@sagikazarmark can you please give this a review? I followed your TODO instructions.

greg0ire · 2016-12-02T09:36:06Z

ping @sonata-project/contributors

core23 · 2016-12-02T10:10:11Z

Formatter/Pool.php

     */
-    public function __construct(LoggerInterface $logger = null, $defaultFormatter = null)
+    public function __construct($defaultFormatter, LoggerInterface $logger = null)


Please add an upgrade note for this.

Maybe also mention that passing null will no longer automatically make the first formatter the default one?

Actually null should be an invalid value in the first place.

Actually null should be an invalid value in the first place.

That's right. I think I'll have to make another PR on 3.x to fix that, and maybe also add deprecation notices for each of the TODOs

Null is valid on the 3.x branch IMO, since it's optional. When it is null, the first one is automatically selected, this is necessary for BC. Otherwise I agree, deprecation warnings should be added. (Actually I am not sure why I didn't add any)

Actually I am not sure why I didn't add any

Probably because this was quite old ;)

I'll remove the null in the signature then

Wait I got confused and removed null from the logger signature. My bad.

Actually null should be an invalid value in the first place.

@sagikazarmark : called cannotBeEmpty to take care of that.

sagikazarmark · 2016-12-02T11:04:48Z

@greg0ire Sure, but I lost the context: what TODO instructions? 😄

sagikazarmark · 2016-12-02T11:05:11Z

Ah, ok, found it

sagikazarmark · 2016-12-02T11:07:06Z

DependencyInjection/SonataFormatterExtension.php

        if (!array_key_exists($config['default_formatter'], $config['formatters'])) {
-            throw new \InvalidArgumentException(sprintf('SonataFormatterBundle - Invalid default formatter : %s, available : %s', $config['default_formatter'], json_encode(array_keys($config['formatters']))));
+            throw new \InvalidArgumentException(sprintf(


I know you are against it, and I am not trying to start a flame war here again, but AFAIK the Symfony CS says exceptions should not be split into multiple lines. Am I wrong?

Fabien said it to me, but I am not sure they put it in their CS rules though (and I think the arguments he gave are not good enough, but can understand that he does not want to have to touch 10000 files over this). And anyway, there is a sprintf here, and we usually split sprintf calls, don't we?

As said, I saw quite a few rounds of debates about this and I am not up to date with the latest decisions on this front, I just wanted to point a possible discrepancy.

Personally, in this case I'll go as far as to prefer discrepancy, so as long as someone does not point me at some docs showing this is part of the sf coding standard, which I think is full of very questionable decisions like Yoda conditions or non-multiline-signatures that are 500 chars long (not kidding, there are things like this in sf), then I'll keep using and asking for linebroken exceptions. It gives better diffs, and people can actually read the exception messages that way.
I don't know why people are so afraid of hitting the carriage return key.

but can understand that he does not want to have to touch 10000 files over this

That's the why

Sometimes I wonder if consistency is that important: if you do something wrong and hard to fix many times, you just give up and carve in your standards : "do it the wrong way"

sagikazarmark · 2016-12-02T11:09:29Z

Formatter/Pool.php

     */
-    public function __construct(LoggerInterface $logger = null, $defaultFormatter = null)
+    public function __construct($defaultFormatter, LoggerInterface $logger = null)


Maybe also mention that passing null will no longer automatically make the first formatter the default one?

sagikazarmark · 2016-12-02T11:11:51Z

Formatter/Pool.php

     */
-    public function __construct(LoggerInterface $logger = null, $defaultFormatter = null)
+    public function __construct($defaultFormatter, LoggerInterface $logger = null)


Actually null should be an invalid value in the first place.

greg0ire · 2016-12-03T14:26:24Z

Formatter/Pool.php

    {
-        $this->logger = $logger;
-
-        // TODO: This should become a required first parameter when the major version is changed


@sagikazarmark ~~I think there is no point anymore to switch the arguments if the logger can't be null, am I right?~~ nevermind that

sagikazarmark · 2016-12-13T19:38:49Z

I guess this needs to be rebased once 3.x is merged into master, right?

greg0ire · 2016-12-13T20:12:28Z

Yes exactly. And reworked a bit also (because of the changes with the logger)

SonataCI · 2016-12-14T01:10:23Z

Could you please rebase your PR and fix merge conflicts?

greg0ire · 2016-12-14T08:13:17Z

@sagikazarmark it's ready, please review !

sagikazarmark

Pure beauty

sagikazarmark · 2016-12-14T14:05:02Z

DependencyInjection/SonataFormatterExtension.php

        if (!array_key_exists($config['default_formatter'], $config['formatters'])) {
-            throw new \InvalidArgumentException(sprintf('SonataFormatterBundle - Invalid default formatter : %s, available : %s', $config['default_formatter'], json_encode(array_keys($config['formatters']))));
+            throw new \InvalidArgumentException(sprintf(
+                'SonataFormatterBundle - Invalid default formatter : %s, available : %s',


Hm, spaces before colons?

I dropped them, it looks weird indeed.

greg0ire · 2016-12-14T14:10:51Z

Pure beauty

😊 thanks!

greg0ire · 2016-12-14T14:19:02Z

@sonata-project/contributors , you already approved, but I changed many things, so maybe a final review before the merge?

sagikazarmark · 2016-12-14T14:20:17Z

TBH I don't understand why approval is not automatically reset when more changes are added to the repo.

greg0ire · 2016-12-14T14:24:07Z

TBH I don't understand why approval is not automatically reset when more changes are added to the repo.

I think it would be hard to get right, because you'd have to separate pedantic changes from significant ones. For instance I just restored UPGRADE-4.0.md that I deleted by mistake, I don't think it warrants a reset.

sagikazarmark · 2016-12-14T14:27:49Z

think it would be hard to get right, because you'd have to separate pedantic changes from significant ones.

Well, I would rather make sure all the time. The same applies to business: you don't deploy ANYTHING, not even a char change to production without approval.

I agree that it could actually be quite uncomfortable in some cases, but that's the only way to keep the formalities.

greg0ire · 2016-12-14T14:39:18Z

I think it depends on the human resources you have on your team. Maybe Github will make this configurable someday?

core23 · 2016-12-14T15:55:38Z

Formatter/Pool.php

@@ -12,14 +12,16 @@
 namespace Sonata\FormatterBundle\Formatter;

 use Psr\Log\LoggerAwareInterface;
-use Psr\Log\LoggerInterface;
+use Psr\Log\LoggerAwareTrait;
 use Psr\Log\NullLogger;
 use Twig_Environment;
 use Twig_Error_Syntax;
 use Twig_Sandbox_SecurityError;

 class Pool implements LoggerAwareInterface


Do we still need the interface when using the trait?

We inject the logger ourselves, so I'd say no… but if we want the class to benefit from some automatic logger injection we're not aware of in some framework, this might be necessary. Doesn't hurt IMO. @sagikazarmark , what do you think?

The Psr\Log\LoggerAwareInterface only contains a setLogger(LoggerInterface $logger) method and can be used by frameworks to auto-wire arbitrary instances with a logger.

LoggerAware can be used as a marker that it requires a log, so I would say keep it. Doesn't hurt.

@core23 @OskarStark please merge this if that's ok with you

core23 · 2016-12-16T11:17:24Z

Thanks @greg0ire

greg0ire added the major label Nov 26, 2016

SonataCI added the review required label Nov 26, 2016

greg0ire changed the base branch from 3.x to master November 26, 2016 16:04

greg0ire force-pushed the drop_old_versions branch from 87a7cc4 to 4d29bea Compare November 26, 2016 16:04

greg0ire mentioned this pull request Nov 26, 2016

Road to Sonata 4 sonata-project/dev-kit#216

Closed

OskarStark approved these changes Nov 28, 2016

View reviewed changes

core23 added the Needs: Upgrade note label Dec 2, 2016

core23 requested changes Dec 2, 2016

View reviewed changes

sagikazarmark reviewed Dec 2, 2016

View reviewed changes

greg0ire force-pushed the drop_old_versions branch from 4fdd829 to 9de44ff Compare December 3, 2016 13:33

greg0ire mentioned this pull request Dec 3, 2016

Add missing deprecations #222

Merged

greg0ire commented Dec 3, 2016

View reviewed changes

greg0ire force-pushed the drop_old_versions branch 2 times, most recently from f4fb543 to b25f76a Compare December 3, 2016 14:33

greg0ire added in progress and removed Needs: Upgrade note labels Dec 3, 2016

core23 approved these changes Dec 4, 2016

View reviewed changes

SonataCI added the pending author label Dec 14, 2016

greg0ire added 2 commits December 14, 2016 08:54

Drop old php versions

55c892d

Drop old symfony versions

87413fc

greg0ire force-pushed the drop_old_versions branch from b25f76a to dd26878 Compare December 14, 2016 08:10

SonataCI removed the pending author label Dec 14, 2016

greg0ire force-pushed the drop_old_versions branch from dd26878 to 314f2b4 Compare December 14, 2016 08:11

greg0ire force-pushed the drop_old_versions branch from 314f2b4 to caf69ed Compare December 14, 2016 08:12

greg0ire removed the in progress label Dec 14, 2016

greg0ire force-pushed the drop_old_versions branch from caf69ed to feec793 Compare December 14, 2016 11:21

sagikazarmark approved these changes Dec 14, 2016

View reviewed changes

greg0ire and others added 2 commits December 14, 2016 15:22

Make the default formatter mandatory

8a84a8a

Remove spaces before colons

a42885b

greg0ire force-pushed the drop_old_versions branch from 86f4b7a to a42885b Compare December 14, 2016 14:22

core23 reviewed Dec 14, 2016

View reviewed changes

core23 merged commit 87d8ee1 into sonata-project:master Dec 16, 2016

Drop old versions #221

Drop old versions #221

Conversation

greg0ire commented Nov 26, 2016 • edited Loading

Changelog

Todo

Subject

greg0ire commented Nov 28, 2016

greg0ire commented Dec 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagikazarmark commented Dec 2, 2016

sagikazarmark commented Dec 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire Dec 3, 2016 • edited Loading

Choose a reason for hiding this comment

sagikazarmark commented Dec 13, 2016

greg0ire commented Dec 13, 2016

SonataCI commented Dec 14, 2016

greg0ire commented Dec 14, 2016

sagikazarmark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Dec 14, 2016

greg0ire commented Dec 14, 2016

sagikazarmark commented Dec 14, 2016

greg0ire commented Dec 14, 2016

sagikazarmark commented Dec 14, 2016

greg0ire commented Dec 14, 2016

Choose a reason for hiding this comment

greg0ire Dec 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

core23 commented Dec 16, 2016

greg0ire commented Nov 26, 2016 •

edited

Loading

greg0ire Dec 3, 2016 •

edited

Loading

greg0ire Dec 14, 2016 •

edited

Loading