In practice this has problems with not-yet persisted models.

Unless you also define all default values for attributes in \Illuminate\Database\Eloquent\Concerns\HasAttributes::$attributes, accessing $newModel->additional_note will bomb.

But this may be legit, e.g. additional_note could be a nullable column so this would mean you would have to define all nullable columns too, etc.

I feel at this stage this might be better a package with a trait (if technically possible).

It sure sounds intriguing though 😄

Btw, I've never had such problems due to the following (I know they can all be counter-argued):

• use of https://github.com/barryvdh/laravel-ide-helper
  I always hear that it "shouldn't be required" for this, yet no one can has given a more practical solution for working with ActiveRecord "the ActiveRecord" way and in practice you don't want to miss this
• use of a static analyzer, e.g. phpstan catches this quite well
• writing tests 😄

In practice this has problems with not-yet persisted models.

This doesn't have any impact on not yet persisted models - it's only when you go to access properties that aren't defined it will error.

Unless you also define all default values for attributes in \Illuminate\Database\Eloquent\Concerns\HasAttributes::$attributes, accessing $newModel->additional_note will bomb.

Yes exactly, it will "bomb" and that's the intended behaviour here. If you've attempted to access a property that hasn't been defined then you could run into weird obscure bugs as outlined in original post. "boming" is exactly how PHP handles accessing undefined properties on a object.

Also consider the fact that trying to call an undefined function on your model also "bombs" - let's flip the coin and suggest that calling undefined functions just silently fails - that too could lead to weird and unexpected behaviour. IMO properties should be just as strict.

But this may be legit, e.g. additional_note could be a nullable column so this would mean you would have to define all nullable columns too, etc.

That's the real trade off here, for nullable columns there will be a bit of overhead to make sure they are defined before being accessed, but the safety here far outweighs this marginal overhead, coupled with the fact that this is an opt-in feature.

It sure sounds intriguing though

👍 😄

As for the comments about php stan and other static analysis tools etc, those are definitely interesting, been meaning to check them out - though I suspect it's very much tied with your IDE/a very particular workflow, and I do question why is it nesscary to have extra tooling for some sensible safety that can be provided out the box?

@mfn can you expand on your comment that this causes problems with un-persisted models?

@taylorotwell

Assume the internal_comment of a post is varchar(100) null in the database (i.e. nullable):

$user = new User();
echo $user->internal_comment; // 💥 

That access will throw the exception.

Basically all attribute access on models not loaded with all columns from the DB are prone to this.

To "fix" this, realistically you have to use strictProperties together with filling all Model::$attributes with their default values; even nullable able ones (but of course you would not fill non-nullable attributes which also have no defaults => they should throw the exception).

FTR I'm not rallying against this feature but it should be known how it behaves in all scenarios. This needs to be added to the docs too.

It breaks the following code:

// Assume we've $relation (another model) loaded _from_ the database with all fields)
$model = new Model();
$model->relation()->associate($relation); // 💥 

The expected behaviour is that after the call to associate():

• $model->relation_id is set to the $relation->id
• $model->relation refers to $relation

However it throws:
Illuminate\Database\Eloquent\PropertyNotFoundException : Call to undefined property [relation_id] on model [App\Model].

Must be due to some internal access to $model->relation_id within associate() somewhere.

In @mfn's example, (new Model)->relation() already throws an exception. This is "caused" by BelongsTo::addConstraints().

@mfn I'm not sure I follow you, but if I do I guess what you're describing is still desired behavior.
If I partially load a model (not bringing all columns) and send the object over to a service class that expects a column that wasn't loaded, it's okay to blow up.
Another scenario; if I instantiate an eloquent model, fill some variables but not all, dont persist the object and send it over to a service class that relies on a field that I did not fill, it's okay to blow up.

This seems to have one caveat though: Eloquent is unable to cast attributes to Value Object, which limits the capability of the Nullable Pattern.
If I have a service class that is capable of handling a missing optional field, I would have to surround the access by a try catch instead of checking if is null. If there are several attributes, each individual attribute would need it's own full block of try catch otherwise I wouldn't know which field blew up.
I guess in that case we would have to recommend not to use the strict attributes. If eloquent ever provides a custom casting, the Null Object Pattern would complement this.

associate() problem

I'm not sure why associate is trying to access $model->relation_id that is a problem that should be addressed in Laravel. It's worth noting that laravel does provide a ->setRelation() which can be a better alternative if your trying to associate non-persisted models.

Throwing in getAttribute() is not easily suppressible

One pretty big problem with throwing exception from getAttribute is it's not easy to suppress using PHP language features, e.g.

$model->getAttribute('some_undefined_property') ?? 'default' // throws exception.
$model->some_undefined_property ?? 'default' // will return 'default'

So what about these proposed changes:

Proposed changes

• Add a new ->getProperty() method - this will get the attribute or throw an exception if not defined, even if $strictProperties is not enabled on the model - this is a good candidate to use in core Laravel code for areas where it absolutely requires a particular property to be loaded in order to function as expected.
• Anytime you are calling a function it will never check for strict property, this includes ->getAttribute(), ->getKey(), etc (except for ->getProperty())
• Whenever you access property directly $model->some_property if it's not explicitly defined it will throw a PropertyNotFoundException (only when strict properties enabled)

With strict mode on, examples:

$model->getAttribute('some_undefined_property'); // null
$model->some_undefined_property; // 💥 PropertyNotFoundException
$model->some_undefined_property ?? 'default'; // 'default'
$model->some_undefined_property ?? null; // null
isset($model->some_undefined_property); // false
isset($model->defined_but_null); // false
$model->getProperty('some_undefined_property');  // 💥 PropertyNotFoundException
$model->getProperty('defined_but_null_property'); // null

Note: this creates improved parity with upcoming version of PHP 7.4 typed properties:

class Invoice {
  public ?string $reference;
}

// $invoice->reference; // 💥
// Throws a fatal error "must not be accessed before initialization" error
// (even though it's nullable, it's not been explicitly defined)

// $invoice->reference ?? 'default'; // 'default'

This creates a clear path not only in your app but in Laravel to be more conscious about accessing properties that may not have even been loaded properly. Remember - this is an opt-in feature to provide added safety.

@deleugpn does the above proposal help with the NullObject pattern? Could you provide an example?

It also breaks MorphTo eager loading and HasManyThrough existence queries:

Comment::with('commentable')->get();

Country::has('posts')->get();

These cases and @mfn's example could be fixed quite easily.

On the face of it I would actually expect this to fail:

IMO, the code is perfectly valid:

$post = new Post;
$post->associate(User::find($id));
$post->save();

Honestly since the risk of this breaking code seems quite high and I personally don't feel like chasing all of that down, I suggest releasing this as a package which ships a base model people can extend instead of the regular Eloquent base model.

@garygreen do you have any further plans following up on the problematic cases far?

I might have come over as counter-interested but in fact I am interested. But as we found out, basic use with eloquent relations is problematic.

I might have come over as counter-interested but in fact I am interested. But as we found out, basic use with eloquent relations is problematic.

Yeah the main issue was Laravel internally relies on the functionality of models to come back with null even for properties not defined on the model.

As Taylor mentioned, we would have to chase down all of those cases in the framework. We would have to decide upon a consistent coding pattern in Laravel which has strict properties in mind.

I outlined a few suggested behaviours in a comment above, so getAttribute() could return null for properties that don't exist, but if you attempt to access directly on the object it'll throw PropertyNotFoundException. If Laravel NEEDS the value of something then we should $model->directly_access the value on the model so we benefit from the safety this PR provides.

Another technical issue was with unit testing. We should test how the model behaves across all unit tests with strictProperties enabled so we can be confident it doesn't break anything internally once enabled.

So yeah, there's still a few technical challenges to address before can benefit from stricter property accessors, I still think it would be a nice addition to the framework.