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.

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

[11.x] Expand Support\Fluent data access and transformation capabilities #53665

Merged
merged 10 commits into from
Nov 26, 2024

Conversation

stevebauman
Copy link
Contributor

@stevebauman stevebauman commented Nov 25, 2024

Description

This PR moves several data access and transformation methods from the Http\Request class, into a new InteractsWithData trait that is now shared with the Support\Fluent class.

This grants us access to it's powerful utility, such as taking a plain array of data and converting it into various types, and checking the state of its keys and values with a simple API.

The InteractsWithData trait requires the implementation of two methods: all and data.

  • all($keys = null) is already implemented on the Http\Request class
  • data($key = null, $default = null) has been made intentionally protected as to not pollute the public API of the Http\Request class with more data access methods and potentially cause confusion to developers

Methods

Below is a list of all methods that have been moved from Http\Request into InteractsWithData.

No breaking changes exist here. All method signatures have been kept the same.

  • has($key)
  • only($keys)
  • exists($key)
  • filled($key)
  • hasAny($keys)
  • missing($key)
  • except($keys)
  • anyFilled($keys)
  • isNotFilled($key)
  • collect($key = null)
  • enum($key, $enumClass)
  • enums($key, $enumClass)
  • str($key, $default = null)
  • integer($key, $default = 0)
  • float($key, $default = 0.0)
  • string($key, $default = null)
  • boolean($key = null, $default = false)
  • date($key, $format = null, $tz = null)
  • whenHas($key, callable $callback, ?callable $default = null)
  • whenFilled($key, callable $callback, ?callable $default = null)
  • whenMissing($key, callable $callback, ?callable $default = null)

Usage

$data = fluent([
    'bool' => 1,
    'int' => '123',
    'float' => '12.34',
    'string' => 'foo',
    'enum' => 'some-enum',
    'enums' => ['other-enum', 'other-enum'],
    'date' => '2024-11-25',
    'array' => ['foo', 'bar'],
]);

$data->bool('bool'); // true

$data->integer('int'); // 123

$data->float('float'); // 12.34

$data->string('string'); // Illuminate\Support\Stringable

$data->enum('enum'); // App\Enum\BackedEnum

$data->enums('enums'); // App\Enum\BackedEnum[]

$data->date('date'); // Illuminate\Support\Carbon

$data->collect('array'); // Illuminate\Support\Collection

Let me know your thoughts! ❤️

@taylorotwell
Copy link
Member

Hey @stevebauman - can you fix the conflict here and mark as ready for review?

@taylorotwell taylorotwell marked this pull request as draft November 25, 2024 23:03
@stevebauman stevebauman marked this pull request as ready for review November 25, 2024 23:09
@stevebauman
Copy link
Contributor Author

@taylorotwell Done! 👍

@taylorotwell taylorotwell merged commit 5e8026f into laravel:11.x Nov 26, 2024
38 checks passed
@shaedrich
Copy link
Contributor

The InteractsWithData trait requires the implementation of two methods: all and data.

I know, I'm overly correct here, but a trait cannot require the implementation of any methods. If you want that, you'd need an interface for that. Not sure if this is just an issue with your wording or there actually should be an interface/contract for this.

@stevebauman
Copy link
Contributor Author

@shaedrich If you add abstract methods on a trait that you do not implement, PHP will throw an exception:

ErrorException: Class [{className}] contains [n] abstract method and must therefore be declared abstract or implement the remaining methods ({className})

This means you are required to implement these methods. You can get mixed up in technical terminology and semantics if you like, of course.

@shaedrich
Copy link
Contributor

shaedrich commented Nov 26, 2024

Touché—but is that the Laravel way of doing things? Especially all could be on an interface, that would be implemented by both Enumerable and InteractsWithData 🤔

@stevebauman
Copy link
Contributor Author

I think that's a good idea. I didn't think of adding it to an interface at the time. Would you submit a PR?

@shaedrich
Copy link
Contributor

Sure, we can try that 👍🏻

@shaedrich
Copy link
Contributor

shaedrich commented Nov 26, 2024

oh, wait, that doesn't make sense as Enumerable doesn't have data() and all() has a $key parameter

Sorry for the confusion—my mistake!

@OzanKurt
Copy link
Contributor

OzanKurt commented Nov 27, 2024

Adding 2 new methods to a very commonly used (extended) class CAN be a breaking change.

Cannot make non static method Illuminate\Support\Fluent::all() static in class App\Enums\Enum

  at app\Enums\Enum.php:14
     10▕     public static array $all;
     1112▕     abstract public static function data(): array;
     13▕
  ➜ 14▕     public static function all(): Collection
     15▕     {
     16return static::$all[static::class] = static::$all[static::class] ?? collect(static::data())
     17▕             ->mapWithKeys(function ($attributes, $key) {
     18$attributes = array_merge(static::getDefaultAttributes(), $attributes);

It also broke a very popular package: https://github.com/yajra/laravel-datatables

image

I think this was a very careless update.

@stevebauman
Copy link
Contributor Author

stevebauman commented Nov 28, 2024

@OzanKurt The addition of new methods is not a breaking change. If this were the case, nothing in Laravel could be updated, as all classes in the framework may be extended.

If other packages have extended Fluent and have conflicts with the new methods, then they must be updated.

@OzanKurt
Copy link
Contributor

I don't believe that you should update a very core class like this, claiming it's not a breaking change and force every developer to update their package or project.

I don't understand why you insist on that it's not a breaking change. Lots of projects and packages are broken since this update.

@shaedrich
Copy link
Contributor

Regardless of breaking changes, it's no secret that Taylor's decision making process is questionable.

@crynobone
Copy link
Member

crynobone commented Nov 28, 2024

@OzanKurt Adding new methods in a class is never a breaking change. That just unfortunate that due to these changes one of the packages you are using is no longer working properly.

@tontonsb
Copy link
Contributor

@crynobone in a literal sense this was a breaking change as it broke existing code. But one can argue that certain breaking changes should be allowed to let the project move forward.

It might be useful and make the project a bit more enterprisey to document what level of BC is guaranteed in minor versions like this Symfony page does.

Btw it looks like Symfony doesn't promise BC with added methods either.

@joshmanders
Copy link
Contributor

I do not use laravel-datatables therefore nothing in my code is broken by using this. Breaking changes only exist on the framework level. If I develop a package that adds functionality to Laravel core stuff and then core also adds functionality that uses similar method names, that is not a breaking change on the framework. That is a breaking change for the package I used and on them to account for it.

Pot shots at Taylor's decision making are also pretty low behavior. If you don't like his choices in his framework, the beauty of open source is that you can fork it and maintain it anyway you like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants