Add support for custom validation messages and attributes when using property morphable

[bentleyo]

Custom validation messages and attribute names don't work properly when using property-morphable data at the moment. They build messages from the base class, not the morphed one, even when the morph attributes are provided.

This PR fixes that by making the DataValidationMessagesAndAttributesResolver class aware of property-morphable data.

There is a section of code I added that is now repeated in both:

• DataValidationMessagesAndAttributesResolver
• DataValidationRulesResolver

Not sure if it should be moved somewhere shared to avoid this duplication.

The code in question:

        if ($dataClass->isAbstract && $dataClass->propertyMorphable) {
            $payload = $path->isRoot()
                ? $fullPayload
                : Arr::get($fullPayload, $path->get(), []);

            $morphedClass = $this->dataMorphClassResolver->execute(
                $dataClass,
                [$payload],
            );

            $dataClass = $morphedClass
                ? $this->dataConfig->getDataClass($morphedClass)
                : $dataClass;
        }

[ncphillips]

We're also running into this issue so I'm very excited for this fix. We make use of polymorphic collections in a couple places and ranging their error messages has been really frustrating.

Edit: If anyone else needs this, you can copy bentley's class into your own app and bind it to the container:

[ncphillips]

Actually, this doesn't seem to be a complete fix @bentleyo

It works if I do MyPolymorphicData::validate([ ... ]) but it does not work if I have a parent data object with a Collection<MyPolymorphicData> property on it.

Will investigate more later

[ncphillips]

Okay, I got a test that reproduced the issue I was seeing, and updated the code to make that test pass.

Opened a PR into this branch @bentleyo

https://github.com/bentleyo/laravel-data/pull/2/files

[bentleyo]

Thanks @ncphillips I'll have a look this evening!

[ncphillips]

We've found a couple other problematic cases.

Case 1: Nullable Polymorphic Child

The first is when an object has a nullable polymoprhic child and it happens to be null:

class Vehicle {}
class Car extends Vehicle {}
class Truck extends Vehicle {}

class Garage {
  vehicle?: Vehicle
}

Note: I opened a PR into this branch to try and fix it, but I've just realized it's failing. bentleyo#3

Case 2: Polymorphic Children of Non-Polymorpch Items in a Collection

The second case is more annoying. We changed our data structure to avoid the problem, but it's still worth mentioning.

If you have an array of non-polymorphic objects, but those objects themselves have a polymorphic child, then laravel-data will not check any further down. For example, if I had a Region that contained various garages:

class Region {
  garages: Garage[]
}

This would fail to use the custom validation messages/attributes for the Vehicles in the Garage

[rubenvanassche]

Thanks for the PR, a few changes are required. @bentleyo Do we need changes to support the cases @ncphillips is mentioning? By the way thanks for the postcard!

[bentleyo]

We've found a couple other problematic cases.

Case 1: Nullable Polymorphic Child

The first is when an object has a nullable polymoprhic child and it happens to be null:

class Vehicle {}
class Car extends Vehicle {}
class Truck extends Vehicle {}

class Garage {
  vehicle?: Vehicle
}

Note: I opened a PR into this branch to try and fix it, but I've just realized it's failing. bentleyo#3

Case 2: Polymorphic Children of Non-Polymorpch Items in a Collection

The second case is more annoying. We changed our data structure to avoid the problem, but it's still worth mentioning.

If you have an array of non-polymorphic objects, but those objects themselves have a polymorphic child, then laravel-data will not check any further down. For example, if I had a Region that contained various garages:

class Region {
  garages: Garage[]
}

This would fail to use the custom validation messages/attributes for the Vehicles in the Garage

@ncphillips sorry I took so long to reply! Thanks for this. I tried to replicate the issue you were running into with Case 2, but it seemed to work for me.

Here's a gist with my replication of the situation you described:
https://gist.github.com/bentleyo/0a8444ed047b84a39b4039105ace81df

Is there anything I'm missing? You can paste this into ValidationTest.php and it should pass.

Update: I misunderstood, I'm able to replicate now! Will look into it.

[bentleyo]

Thanks for the PR, a few changes are required. @bentleyo Do we need changes to support the cases @ncphillips is mentioning? By the way thanks for the postcard!

@rubenvanassche sorry for the delay!

I'm glad you got the postcard 🎉 I checked the Spatie postcard wall website a few times after the initial failed send and I thought it must be ended up in the wrong place again 😂 I hope the postcard inside-an-envelope, inside-another-envelope gave you a laugh. It's really not the way a postcard is meant to work is it?!

I've completed the changes you requested. I was unable to replicate the second situation @ncphillips mentioned, but I might be missing something. (update: I'm looking into it)

[bentleyo]

@rubenvanassche I've updated the code to handle the case @ncphillips mentioned (property-morphable data inside an array of static data).

The problem was that we were trying to determine the property-morphable data class from a path like example.*.data when property-morphable requires the actual payload.

To tackle this problem I implemented an approach that converts wildcard paths to static paths when a property-morphable data class is encountered (e.g. example.0.data, example.1.data). The reason I took this approach instead of iterating over every possible path is because I thought it might be more performant. Open to any suggestions or ideas here.

I implemented it in a way that should work with deeply nested wildcard path conversion. Let me know if you have any questions or think of any potential problems!

[bentleyo]

@rubenvanassche happy new year mate! When you get the chance could you please look over this one for me?

[bentleyo]

@rubenvanassche is there anything needed on this PR before you'd be happy to consider this one?

I just ran into this problem again and had to add a temporary workaround in the parent of the property-morphable data class.

[rubenvanassche]

Hi @bentleyo, working atm hard to get TypeScript Transformer 3 out, when that's done I'm gonna commit working myself through all the data issues and PR's but since both packages are so complex I don't want to mix them up while working