search

markhuot / craft-pest

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

Http refactoring #3

Closed ostark closed 2 years ago

ostark commented 2 years ago

I was wondering if you are open to more structural changes, so I moved my idea to this PR. This is the commit I'd love to get some feedback

👉️👉️👉️ https://github.com/markhuot/craft-pest/pull/3/commits/b53529d4f92d3c6562520f5d52eb3f3cc0152a20

Not sure if it is already working, it's actually fully untested! It's more a question of the design.

The interface/DX did not really change:

Here you can see the entry point - it should help you to follow the code: https://github.com/ostark/craft-pest/blob/b53529d4f92d3c6562520f5d52eb3f3cc0152a20/src/test/TestCase.php#L86-L100

ostark commented 2 years ago

Please let me know if you are happy or uncomfortable with the changes.

I know this is fairly opinionated, the code looks very different than other Craft plugins. I'm open to discuss it. It's even okay if you fully reject it. I just need a signal from you.

ostark commented 2 years ago

Ping

markhuot commented 2 years ago

I hope it's okay I fixed up a few of the things I left comments on. Also, there's a new src/bin/craft that should be executable. It'd allow you to run ./src/bin/craft install to get a local install going directly in this repo. You can also run ./vendor/bin/pest directly in this repo (after installing) and you should get some passing tests. There's some field layout issues that need to be fixed up on the factories (which I'll do) but the request/response stuff is all working.

It'd be great to get some tests in there for the GET and POST requests and all the various permutations on them.

image

markhuot commented 2 years ago

I fixed up the failing tests too. Factories should work for generating fields, sections, and entries again. Still working on the API for matrix blocks.

markhuot commented 2 years ago

Woo! ./vend/bin/phpstan and ./vendor/bin/pest are now running on GitHub actions for this PR (and will for master after merge).

All tests are currently passing for this PR.

ostark commented 2 years ago

Glad you like it! I suggest to backport theses changes to Craft 3 in a nother branch and continue with the further development for Craft 4 only. Or we use rector for compatibility.

I'll review your comments when I'm back.

markhuot commented 2 years ago

Yup, that makes sense. As I got in to it some of the field/field layout stuff in the factories is pretty different between 3 and 4 so I think keeping master targeted to the latest makes sense.

markhuot commented 2 years ago

I'm going to merge this so I can work on some other PRs against this new approach.