tpetry
[DRAFT] feat: support functions
This PR adds support to create functions. We can of course change the signature but this is just a first proposal because I think it would be a great addition.
I like the idea how you added support for all the optional modifiers but don't like the mixing of int-indexed and assoc-indexed array when combining the language and more options:
[
'language' => 'sql',
'parallel safe'
]
How about making it more phpstan friendly by using only assoc indexed modifiers? Is there one which would not work?
Like:
[
'language' => 'sql',
'parallel' => 'safe',
'leakproof' => false,
'mutability' => 'stable',
]
Yep, that's what I originally thought as well but I wasn't sure if we can really support all of the modifiers eventually:
However, I just pushed another change where all the different possibilities are handled. Let me know what you think 😊
Give me some time with both pull requests. They are on my agenda for a long time and I didn't find a good way yet to express all the PG functionality and make it good to use. I will have to play with them and try to remember the problem I had foreseen for my implementation.
Sure and thanks for considering both of them. I already use this one using my own extension so I thought it might be a good fit for this library as well.
https://jkatz05.com/post/postgres/postgres-begin-atomic/
Using the atomic by default for PG 14 sounds like a good idea. What do you think @jaulz?
Oh, that looks super cool, @tpetry. How would you implement the version check? Calling SHOW server_version; and parse it?
Yes, i did the same: https://github.com/tpetry/laravel-postgresql-enhanced/blob/d557d33bcddd635f3ba4af46c7eaefd61a60c348/src/Query/BuilderExplain.php#L14-L20
I just noticed that the atomic keyword is part of the body (i.e. after BEGIN). However, I just pushed a change that "smartly" injects the atomic keyword. Let me know what you think.
I will take some time on the weekend to play with the PR, I didn't forget it ;) I just have very much to do currently.
No worries and thanks for spending your time at all on it! By the way, I think once we have this we could also think about creating utilities for triggers and maybe even provide Laravel specific triggers (e.g. create trigger to cascade deletions of morphable relations).
I've improved your PR to fit more with the rest of the library and changed the implementation to correctly use the atomic mode. I decided to use it by default for PG14+ as it is a good safety feature.
But I don't like the current option for the null handling. calledonnull: bool doesn't feel correct, for every other term I was able to find their correct naming in the documentation (e.g. they use volatility instead of mutability).
It's missing manual testing for all options currently and I need to create about 50 more unit tests for the final implemenation.
What's your opinion?
That looks great! I am just wondering whether calledonnull (I would actually prefer calledOnNull) can be described as boolean because that part is actually defined as { CALLED ON NULL INPUT | RETURNS NULL ON NULL INPUT | STRICT } (i.e. STRICT is a third option?).
Would you create tests for all the possible combinations? If yes, I think it could be handy to use phpunit's @dataProvider and just create one test using a data provider which provides the input and the expected output.
The strict mode is just an alias for the RETURNS NULL ON NULL INPUT mode, so in the end it's only two modes. But I still don't like the calledOnNull name, will have to think about it.
I won't create a test for every combination, that is just too much. Just for every single one, but thats still a lot of tests.
I was just about to ask if you need a hand to implement the tests but saw that are you already implemented a couple of them. Are any tests missing from your side where I can help directly?
I am almost finished, will need to review it again and write the documentation. Will finish everything this week ;)
It's been released with 0.20.0. Thanks for your pull request.
It was a lot of work getting this fully done but without your draft, I wouldn't have had the time. Sorry for being so slow.