[5.1] Example of recently inserted state tracking

[atrauzzi]

Fairly trivial, but I think this could be helpful in many scenarios.

Closes #9745.

[Anahkiasen]

That would be very useful

[atrauzzi]

And of course, anyone is open to suggest a better name for the variable ;)

[brunogaspar]

Minor indentation issue here

[brunogaspar]

I like the idea.

[taylorotwell]

What were the use cases for this?

[deefour]

If I understand correctly, this would help for places we don't want to hook some created event in an observer; where inlining the conditional functionality is just fine. #9745 outlines an entirely different use.

function persist(Podcast $podcast, array $attrs) {
  $podcast = $podcast ?: new Podcast;

  $podcast->forceFill(array_only($attrs, [ 'title', 'url' ]));

  $podcast->save();

  if ($podcast->recent) {
    // do something with $podcast that required it
    // already being saved to the db, but only when
    // it was just created.
  }
}

Setting a flag up front is currently necessary.

function persist(Podcast $podcast, array $attrs) {
  $podcast  = $podcast ?: new Podcast;
  $creating = ! $podcast->exists; // Need to set a flag here; this PR makes this unecessary

  $podcast->forceFill(array_only($attrs, [ 'title', 'url' ]));

  $podcast->save();

  if ($creating) {
    // ...
  }
}

[atrauzzi]

Without functionality along these lines, repetitive blocks using firstOrNew are required at a very high layer. Which I feel is a lot of defensive coding for metadata that has nowhere else to live.

In our case, we need it to respond with HTTP 200 or 201. So with this feature, we could do things like this:

return new JsonResponse($model, $model->recent ? 201 : 200);

However, producing the same state without having Eloquent accumulate it (similar to exists) would force the implementation to do all data manipulation at the same level as the check -- or require an ugly sidechannel for the extra metadata, causing unfortunate design problems:

$data = Model::firstOrNew($attributes)

if($data->exists) {
    $recent = false;
}
else {
    $recent = true;
    $data->save();
}

return new JsonResponse($model, $recent ? 201 : 200);

Which is not ideal as the state has to be inferred manually at the time the store/update functionality is used. While it produces the same end result, firstOrNew and JsonResponse shouldn't be required to exist at the same layer.

As I mentioned earlier, if you want $recent anywhere outside of where this check occurs, you have to somehow bundle it with the model. Which hopefully justifies having the model track that state internally as a conveneince.

:)

[lukasgeiter]

I think the variable could be a bit more self-explanatory. What about recentlyCreated?

[atrauzzi]

@lukasgeiter - I'd like to see if there is a different name that is more specific, although it might be nice to keep it as short as possible. Let's wait for as many suggestions as we can get :)

[GrahamCampbell]

This feature would be really useful to me too. 👍

[GrahamCampbell]

My use case is working out if a user has signed up.

[dtaub]

What about new for the variable name? It's concise and (imo) better conveys the fact that the record was newly created. recent almost sounds like it's checking for some arbitrary age, whereas new implies it was created just now on the current request.

[Anahkiasen]

Isn't new a reserved keyword though?

[GrahamCampbell]

Isn't new a reserved keyword though?

No, not for properties. I tried it out too just to be sure.

[Anahkiasen]

Then in that case I'd prefer new too I think, better conveys the intent than recent

[lukasgeiter]

👍 for new. But what would its initial value be when you create a fresh model instance?

[GrahamCampbell]

Yeh.

[dtaub]

@Anahkiasen Yeah, I was thinking it might be reserved, too, but double checked before posting. The only issue that crossed my mind was that to someone used to Ruby, it might kinda look like it's creating a new instance, but that's kind of a stretch.

[arrilot]

I would have gone even further - isNew because we already have a verb exists

if ($model->exists) {
...
}

if ($model->isNew) {
...
}

[dan-har]

I think it is bad to add more public properties to the model, it can make conflicts with the dynamic properties, for example if i have a column 'new' or 'recent' inside a table. It should be a protected property that is accessed by a public method.

[dtaub]

Yeah, it would be pretty standard to have a protected property new and access it using isNew(). No point in making it public.

[atrauzzi]

@GrahamCampbell @taylorotwell -- Let me know what you guys would prefer to see and I'll amend my MR.

[GrahamCampbell]

@atrauzzi It's totally up to @taylorotwell. :)

[atrauzzi]

For sure, @taylorotwell - Whatcha think? :)

[taylorotwell]

Renamed it to wasRecentlyCreated to make it a little longer and less likely to collide.

[atrauzzi]

👍 Awesome!

[hauthorn]

I'm not sure to write this here.
This addition does make some testing using Laravels "assertViewHas" fail tests.

See my gist here for an explanation: https://gist.github.com/hauthorn/fb1494c3ba5238283acc

This didn't fail before. Now it does.

Do I use the "assertViewHas" in some inappropriate way, or is this an unintended consequence of the change?