Skip to content

Commit

Permalink
formatting
Browse files Browse the repository at this point in the history
  • Loading branch information
@taylorotwell
taylorotwell committed Apr 16, 2018
1 parent 4ba828b commit 27b8844
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 11 deletions.
2 changes: 1 addition & 1 deletion composer.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
"erusev/parsedown": "~1.7",
"league/flysystem": "^1.0.8",
"monolog/monolog": "~1.12",
"nesbot/carbon": "^1.24.1",
"nesbot/carbon": "1.25.*",

This comment has been minimized.

Sign in to view

Copy link
@kylekatarnls

kylekatarnls Apr 16, 2018 •
edited
Loading

Contributor

I recommend to upgrade to 1.26 instead and remove overrides: #23894 so there will be no gap anymore between Carbon and Laravel's Carbon and no more compatibility issues. If Laravel needs new Carbon features in the future, we would be happy to merge it in the Carbon project, or else propose a solution via some mixin/macro to avoid overriding solution.

This comment has been minimized.

Sign in to view

Copy link
@GrahamCampbell

GrahamCampbell Apr 16, 2018

Member

Why do they keep making breaking changes. They should just stop doing that, or release a major version each time. :/

This comment has been minimized.

Sign in to view

Copy link
@Miguel-Serejo

Miguel-Serejo Apr 16, 2018

Carbon doesn't claim to follow SemVer. I find it somewhat ironic that Laravel assumes its dependencies follow SemVer when it doesn't. Also that we're complaining about breaking changes in other libraries when breaking changes are routine in laravel's release cycle.

This comment has been minimized.

Sign in to view

Copy link
@kylekatarnls

kylekatarnls Apr 16, 2018 •
edited
Loading

Contributor

If I may return the question why Laravel first overridded Carbon instead of proposing PR in the Carbon project?

Adding a protected variable is not a breaking change. If a class never can add any method, any argument and any property because someone somewhere can use the same name, then you never update (or always tag major version, that would have no sense).

Extend a class enter the protected level and it's taking some risk on your own. The same could happen on any library. We add public methods and protected members in a way that respect semver (at least for 1.26).

Carbon is not dedicated to Laravel only, and we cannot renounce to implement macros just because you did it before us, sorry. Now that the features Laravel needs are in Carbon, you can drop overrides, so such conflicts will no longer happen.

PS: I do my best to keep Carbon Laravel-friendly. I always test first Carbon future versions on my Laravel projects and did not get into any trouble for 1.26.0 on my machine. I still don't know why some users had this failure and not other ones. But I really want to make Carbon compliant with Laravel's features and upgrade it to match the Laravel's philosophy. So users of Laravels and other frameworks will all be able to rely on our documentation and library that implement Carbon stuff to be compatible with all Carbon sub-classes and not only the Illuminate one. That's why we needed to implement macros on our own.

This comment has been minimized.

Sign in to view

Copy link
@GrahamCampbell

GrahamCampbell Apr 16, 2018

Member

Adding a protected variable is not a breaking change. If a class never can add any method, any argument and any property because someone somewhere can use the same name, then you never update (or always tag major version, that would have no sense).

Actually it is, if the class is not final.

This comment has been minimized.

Sign in to view

Copy link
@kylekatarnls

kylekatarnls Apr 16, 2018 •
edited
Loading

Contributor

I agree with @36864 about Laravel versioning, I had migrating work to jump from 5.3 to 5.4, 5.5 and 5.6 and I don't complain about it. Those efforts was needed to benefit new Laravel's features and I'm so happy to get them. I spend a lot of my spare time to provide new features on Carbon too and I believe those features worth the small adjustment needed in Laravel (#23894). I hope you approve the idea of having the less overrides possible in your classes that extend external libraries.

This comment has been minimized.

Sign in to view

Copy link
@Miguel-Serejo

Miguel-Serejo Apr 16, 2018 •
edited
Loading

If I may return the question why Laravel first overridded Carbon instead of proposing PR in the Carbon project?

This is on par with asking why anyone would extend a laravel framework class (such as the Model class) for their project instead of proposing a PR. When you supply a library that provides non-final classes, you must accept that they can be extended by the user. (Also Carbon was effectively dead for quite a while with PRs sitting there for over a year)

Anyone (not just Laravel) who decided to extend the Carbon class in their project potentially had their code broken, not because they decided to override something in the base class, but because they added functionality that now exists in the base class.

SemVer isn't very clear about what actually constitutes a "public API" or whether protected elements of a non-final class are private or public. It also falls short of contemplating extensions to your classes. Personally, I would consider anything in your library that can be extended part of the "Public API", so any changes, additions, or removals of class members should be considered a breaking change. Past events have revealed that my personal opinion on what constitutes a breaking change isn't necessarily shared by many people, but I still believe it would prevent many problems at the simple cost of increasing a major version number rather than a minor version number in some cases. A high major version number is nothing to be feared.

I do appreciate the work you've put in to breathe life back into Carbon. I also understand that sometimes, its difficult to determine what is or isn't a breaking change. I would ask that you take into consideration the fact that Carbon is an extensible library, so some extra care should be had when making changes.

As a final note, if Carbon now offers everything that Laravel needs from it, I would support the move to remove Support\Carbon. It would have to happen in 5.7, and be documented as a breaking change, but I am wholly in favor of it.

This comment has been minimized.

Sign in to view

Copy link
@kylekatarnls

kylekatarnls Apr 16, 2018 •
edited
Loading

Contributor

If adding a method is a breaking change, then, nearly every single Carbon release will be a major release (as for Laravel releases) and so frameworks and users will have to upgrade manually at any change, it seems very overkill. So OK, it will prevent breaking sub-classes but it will also bother many users who like the automated updates.

And last thing, I did not tag this release lightly, I tested those macro stuffs, in multiple Laravel projects (requiring dev-master) for several weeks (it's on dev-master for 27 days). And I had no troubles. I updated a project just after the 1.26 release, check that it was the right version installed and the same with create-project and all those tests passed successfully. I can't explain why you get troubles and not me.

This comment has been minimized.

Sign in to view

Copy link
@kylekatarnls

kylekatarnls Apr 19, 2018 •
edited
Loading

Contributor

@taylorotwell This should not have been merged and tagged in 5.6, it causes regression for app that require carbon: dev-master or carbon: ^1.26 and apps that use 1.26 stuff. Moreover this should not be hidden in a formatting commit as it has a functional impact.
"psr/container": "~1.0",
"psr/simple-cache": "^1.0",
"ramsey/uuid": "^3.7",
Expand Down
20 changes: 11 additions & 9 deletions src/Illuminate/Routing/PendingResourceRegistration.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,6 @@ class PendingResourceRegistration
*/
protected $registrar;

/**
* The resource's registration status.
*
* @var bool
*/
protected $registered = false;

/**
* The resource name.
*
Expand All @@ -39,6 +32,13 @@ class PendingResourceRegistration
*/
protected $options = [];

/**
* The resource's registration status.
*
* @var bool
*/
protected $registered = false;

/**
* Create a new pending resource registration instance.
*
Expand Down Expand Up @@ -150,15 +150,17 @@ public function middleware($middleware)
}

/**
* Register the Resource.
* Register the resource route.
*
* @return \Illuminate\Routing\RouteCollection
*/
public function register()
{
$this->registered = true;

return $this->registrar->register($this->name, $this->controller, $this->options);
return $this->registrar->register(
$this->name, $this->controller, $this->options
);
}

/**
Expand Down
4 changes: 3 additions & 1 deletion src/Illuminate/Routing/ResourceRegistrar.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,9 @@ public function register($name, $controller, array $options = [])
$collection = new RouteCollection;

foreach ($this->getResourceMethods($defaults, $options) as $m) {
$collection->add($this->{'addResource'.ucfirst($m)}($name, $base, $controller, $options));
$collection->add($this->{'addResource'.ucfirst($m)}(
$name, $base, $controller, $options
));
}

return $collection;
Expand Down

0 comments on commit 27b8844

Please sign in to comment.