markhuot / craft-pest

https://craft-pest.com
Other
38 stars 11 forks source link

Improve Factories #15

Closed ostark closed 1 year ago

ostark commented 2 years ago

It's actually the first time I had a look at your Factory implementation.

I noticed a few things:

Types / auto-completion

I'm blind without ide autocompletion. Not sure if we are able to attach custom fields properly to the Elements using generics It's probably impossible to get auto-completion on "field attributes" without generating a stub for each EntryType, but the actual model properties and methods should be accessible on Section, Entry, Field, etc ...

createElement() vs createModel()

Most of the factories are for Elements, but some are not: Field, Section, and Volume - those are plain models. For the user/developer it does not make any difference, but for internal clarity, I'd use createModel().

Craft 4 vs 3

Honestly, Idk if it is better to maintain two versions or to live with the if-else switches. I just noticed the current approach creates a lot of clutter.

markhuot commented 2 years ago

I'm blind without ide autocompletion. Not sure if we are able to attach custom fields properly to the Elements using generics It's probably impossible to get auto-completion on "field attributes" without generating a stub for each EntryType, but the actual model properties and methods should be accessible on Section, Entry, Field, etc ...

Yup, I'm adding in @method docblocks for those as I come across them. I need to take some time and just mass generate them, I suppose. Or maybe there's a way I can @mixin them but I'm not positive.

Most of the factories are for Elements, but some are not: Field, Section, and Volume - those are plain models. For the user/developer it does not make any difference, but for internal clarity, I'd use createModel().

Yup, that makes sense. I originally started it with Entry elements and then later added Field/Section so that's the reason for the bad naming.

That said… adding a Field is actually not a rollback-able event in MySQL because it performs an ALTER TABLE which has an implied COMMIT on all existing transactions. I'm debating removing the Field factory for that reason.

Honestly, Idk if it is better to maintain two versions or to live with the if-else switches. I just noticed the current approach creates a lot of clutter.

Yea, I'm watching this… I don't hate it yet, but I also don't love it right now either. Open to other ideas or a better place to put that forking logic.

markhuot commented 2 years ago

All that said, are you having luck with factories? I have to admit that they are my favorite part of the entire plugin. This just makes me giddy,

// Make 5 posts
$posts = Entry::factory()->section('posts')->count(5)->create();

// Get the /posts page
get('/posts')

// Get all the post titles on the page
->querySelector('.post-title')

// Start an expectation
->expect()

// Expect the post titles to match the posts generated above
->toEqual($posts->pluck('title')->toArray());
ostark commented 2 years ago

That said… adding a Field is actually not a rollback-able event in MySQL because it performs an ALTER TABLE which has an implied COMMIT on all existing transactions. I'm debating removing the Field factory for that reason.

Without Fields, Factories would be kind of useless for testing real-world sites. There must be a workaround.

ostark commented 2 years ago

adding a Field is actually not a rollback-able event in MySQL because it performs an ALTER TABLE

I think I need a test or two to fully understand the issue.

ostark commented 2 years ago

I have to admit that they are my favorite part of the entire plugin.

I like them too! That's why I'd love to see them in a different package (I mentioned it earlier).

myleshyson commented 2 years ago

@mixin ElementTrait, @mixin FieldTrait, @mixin CustomFieldBehavior get you pretty far for base element, field, and custom field properties. Every specific element type and model though would need to have their properties annotated manually. There's not a ton of properties outside of ElementTrait and FieldTrait, so maybe that's doable?

Other option would maybe be to have a console command devs can run to autogenerate compiled classes for any class that extends factory suing the newElement method as a guide. Definitely more complicated but takes the stress out of syncing properties after every Craft update.

On a different note newElement might need to be renamed to newModel or something like that.

markhuot commented 2 years ago

Yea, I'm planning on looking in to this soon-ish. I was going to see how much work it would be to generate those stubs myself per-element type. If that proves too much work then reusing the close but not perfect existing mixins may be enough.

Also, yeah, newElement should definitely become newModel or even newComponent as I think Yii calls it's base components. Hindsight is 20/20 as they say. I really didn't expect to enjoy using factories as much as I do. I thought I'd use them for some entries here and there. But once I started creating sections and fields for testing it got so much more powerful.

ostark commented 2 years ago

+1 for a command to support only the fields that belong to the Model

Feel tree to steal this, otherwise I'll do it https://github.com/ostark/craft-prompter/blob/master/src/Services/ElementModelWriter.php

https://github.com/ostark/craft-prompter/blob/493da17c3bb3bca18daebdeaf44f04acfc981fd1/tests/__snapshots__/ElementModelWriterTest__file_content_matches__1.txt (It's something similar as far as I remember)

markhuot commented 1 year ago

With #89 merged I think I can finally close this issue as "resolved" for now. I'm using the updated type hints in a large project with hundreds of fields and they're all type hinted correctly throughout the tests. I can look at future improvements like scoping fields to their section but, honestly, Craft doesn't do this itself so I'm not so sure Craft Pest should do it either.