[DRAFT] feat: support functions

[jaulz]

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.

[tpetry]

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',
]

[jaulz]

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 😊

[tpetry]

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.

[jaulz]

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.

[tpetry]

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?

[jaulz]

Oh, that looks super cool, @tpetry. How would you implement the version check? Calling SHOW server_version; and parse it?

[tpetry]

Yes, i did the same:

laravel-postgresql-enhanced/src/Query/BuilderExplain.php

Lines 14 to 20 in d557d33

	$version = $this->getConnection()->selectOne('SHOW server_version')->server_version;
	$options = match (true) {
	$analyze && version_compare($version, '13') >= 0 => 'ANALYZE TRUE, BUFFERS TRUE, SETTINGS TRUE, VERBOSE TRUE, WAL TRUE',
	$analyze && version_compare($version, '12') >= 0 => 'ANALYZE TRUE, BUFFERS TRUE, SETTINGS TRUE, VERBOSE TRUE',
	$analyze => 'ANALYZE TRUE, BUFFERS TRUE, VERBOSE TRUE',
	version_compare($version, '12') >= 0 => 'SETTINGS TRUE, SUMMARY TRUE, VERBOSE TRUE',
	default => 'SUMMARY TRUE, VERBOSE TRUE',

[jaulz]

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.

[tpetry]

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.

[jaulz]

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).

[tpetry]

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?

[jaulz]

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.

[tpetry]

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.

[jaulz]

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?

[tpetry]

I am almost finished, will need to review it again and write the documentation. Will finish everything this week ;)

[tpetry]

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.

[jaulz]

Very cool, thanks a lot for your hard work and no worries! 😊