Isn't item_id an int in the database?

That's correct , the code is a bit messy. It uses item_id to store both item number and id. So it can be string or int. I can rename the variable to make that clear

OK that makes sense. another way, in addition to renaming is to make the data type of the parameter a union type. IMO this makes it clearer that both types are accepted. e.g., foo( string|int bar): bool

Also I think we said that as we modify the code we are going to refactor function and variable names from snake_case to camel case for PSR-12 compliance. Are you up for that refactor in these few sections of code?

I just checked PSR12 and it recommends to have function names in camelCase, but there is no recommendation for variables.

PSR12 does not explicitly require camelCase variable names, and when referencing database fields, it's not even possible, but I argue that where possible, we should agree to variable names using camelCase for consistency with the naming convention for methods. A second argument is that this matches CI4 coding conventions so all the files we inherit from CodeIgniter will be camelCase variables. A third argument is a guess that I haven't verified, but I won't be surprised if IDE linters complain about variable names not being camelCase.

I know this isn't the same as a specific declaration, but are there any variable names in the PSR12 OR PSR1 example code that show camelCase or snake_case?

@objecttothis fine by me to change it, but I would try to use an AI to do this conversion. currently I am a bit time constrained and wanted to get this issue fixed. Let's log a ticket for the conversion so it can be tackled after.

I would like to release a 3.4.1 soon as many fixes made it to master already. would be nice to have the mysql one as well. Then we can do more cleanup after

That is fine to punt it to another PR.

I would like to release a 3.4.1 soon as many fixes made it to master already. would be nice to have the mysql one as well. Then we can do more cleanup after

Without the migrations fixes that I have a PR out for, anyone running MySQL will still not be able to upgrade. I'm almost done with my PR. I'll try to get it to a stopping point tomorrow but I could really use someone testing the changes with MariaDB. The only two changes I have left are to add the migration file to kick off one of the migration SQL's and then test my code changes to make sure we can still delete a dropdown. I'm traveling June 9 - August 20, so I too want to get this into a good place to release 3.4.1, but I don't think we should release anything that MySQL users can't install.

Agree let's try to get this sorted. If you want just push the changes to a branch here in the repo and it will auto deploy. Then we can call for some testing. We use Mariadb for the dev server so it will be clear if it starts correctly from scratch or not. It runs all the migrations after every deploy so that is a good starting point. Still need to give you acces to it at some point.

Sounds like you will have some nice time oiff then June onwards, where are you heading?

Agree let's try to get this sorted. If you want just push the changes to a branch here in the repo and it will auto deploy. Then we can call for some testing. We use Mariadb for the dev server so it will be clear if it starts correctly from scratch or not. It runs all the migrations after every deploy so that is a good starting point. Still need to give you acces to it at some point.

Sounds like you will have some nice time oiff then June onwards, where are you heading?

My family and I are headed to the US to see family and friends. I'll have a bunch of other work to do mixed with some fun things like a backpacking trip, but I won't have time to write code. I'll try to answer questions where I can though. #4230 is ready for review and if all is good, we can get both these merged. In the time I have left before travel, I'll probably start work on the framework for CodeIgniter Events in our code which will pave the way for 3rd party integrations.

Agree let's try to get this sorted. If you want just push the changes to a branch here in the repo and it will auto deploy.

I changed #4230 from a draft to a ready pull request and pushed changes, but it says it hasn't deployed. I wonder if because you had requested changes a few weeks back it doesn't deploy.

Ah no the github status is not linked to the dev server deployment, I never looked into that in fact. The deploy is done when travis pushes the final docker image for a commit. It uses a webhook in docker hub if I'm not mistaken.
You can check which commit is deployed by clicking the version string in the footer on the dev server.

cae921 Currently this commit is deployed, but it's on MariaDB. The migrations seem to have ran

I have pushed the commit for this branch again and reworked a bit by looking at how this work for sales. It's similar now so more consistent. I tried to add both an id and item_number and both worked fine. I'll merge this and then we can release, unless if someone finds another issue.
Thanks for driving the last open work home @objecttothis not so long before your time off.

@objecttothis so it's ready for review, if you want.

@objecttothis so it's ready for review, if you want.

I'll take a look

It's quite cool, I asked copilot to convert the method to camelCase, gave me the converted version. Also found that VSCode might be able to go one step further and there you can integrate an AI agent that can then even do the update itself. The plugin is called cline.

Copilot can be used as a linter also in this case, I asked it to check the whole file and it gave me quite some suggestions

It's quite cool, I asked copilot to convert the method to camelCase, gave me the converted version. Also found that VSCode might be able to go one step further and there you can integrate an AI agent that can then even do the update itself. The plugin is called cline.

Does it only refactor the local variables or does it also refactor function names? I've been nervous to ask AI to do the refactoring out of fear that it won't refactor the function definition or all the usages. That and PHPStorm seems to do a pretty good job at the refactoring... Just not automatic.

Indeed it renamed the function but not the references to it. In this case that was not much of an issue as it was one function only