I have tested this item ✅ successfully on cf315fa

I confirm the issue and the fix, also there should not be any side-effects on removing comma from the type name as there are no names with comma

Also this fix applies for cases: FLOAT(M,D), DOUBLE(M,D) too

Doesnt the same change have to be made in other drivers? I see the same regex there as well

I don't know if I agree with this change, or the existing behavior. If we're just getting the data type, then I suppose this is OK, but the way I read the field definitions for the fixed point types the numbers are part of the field type definition so just stating that the field is a FLOAT or DECIMAL (in the case of MySQL) is really only good for trying to typecast the field values to a specific format, which doesn't happen anywhere in Joomla.

I don't know if I agree with this change, or the existing behavior ... the numbers are part of the field type definition

Ok, yes, you are right
but this PR is not relevant to the above,

its purpose is not to change existing behaviour of the method for method parameter:
$typeOnly = true
that would be a B/C break, right ?

• so since we are not changing the existing behaviour, and since we already removing the number and parenthesis from the type name, then the comma should go too, thus only problem of this PR is to care for the other DB drivers too ?

Furthermore if some extension needs the full type of the column, then it is already possible:

• you give to the method: $typeOnly = false thus you get the -full- objects into the result array and then in each of the objects you can use column->Type to get the full type of the column

In the case of the fixed point types though, is returning those WITHOUT the number definitions actually a proper type definition or are we returning only a partial definition? I think what we're doing falls under the latter; there's a difference between stripping the max length (either in characters or bytes based on the column type, i.e. varchar(255) or int(11)) and stripping part of the actual field definition.

... actually a proper type definition or are we returning only a partial definition? ... stripping part of the actual field definition

ok i see now 1 more reason

• that original choice of returned data was not best

If you want to deprecate the method so that it is made more proper,
that is a different topic

Any objection that this PR is just fixing the existing behaviour without a B/C break ?

What's the correct behavior though? I'm not so much concerned about existing behavior. What are we considering a type compared to what the SQL engine considers a type? Something that can map to a PHP scalar type (int, bool, float, etc.) or the actual type definition of the field? In the case of fixed point types is DECIMAL(10,5) the full type definition or is the type definition only DECIMAL? If the type is without what's in the parentheses always then this PR is fine as is, but for those field types if the type requires what's in the parentheses (based on the SQL engine's definition, not a PHP type) then to me this is wrong and the correct fix is special handling for those fields to ensure data isn't stripped.

@mbabker:

It suprises me that you "don't know if you agree with this change, ...", since it is a bug fix. I agree with Georgios that this PR does not concern the validity of the behavior, which is a whole different topic.

I find the argument "..., which doesn't happen anywhere in Joomla" equally surprising. After all, the method is part of the public Joomla! API and can be used anywhere and not just by Joomla! core. I use this method exactly for the thing you mention: "trying to typecast the field values to a specific format" :) So I'm the one to blame for this PR ;)

I want to make sure it's a correct fix. And yes, that does mean raising question to the validity of the existing behavior. If what we are doing when we return a type produces invalid types by some arbitrary definition, then we need to change our code to ensure types are correct. So I'm asking now what the API design is supposed to be; if types only is requested are we returning a PHP type or a SQL engine type, and why is the type value different based on the boolean passed to this method?

To me it seems like there should be two methods the longer I think about this; one returning schema info (which is what the existing method is doing with boolean false) and one returning PHP compatible types (which it seems like is trying to happen when you pass boolean true to the existing method). Then regardless of whether you pass true or false into the existing method you are consistently getting the same value for the field type, then the flag just instructs the API to return a subset of the available data.

I agree completely with you on the questionable usability of this method. But that's not the point of this PR. This PR simply fixes a bug. In my opinion the purpose of the preg_replace() is quite obvious. It's meant to remove anything between and including the brackets following the type name. I think it's equally obvious that the one who crafted this piece of code, simply overlooked the decimal type. So in my humble opinion, this PR's aim simply aims to fix that omission and not to improve on the usability of the method.

That's where I apparently disagree with others. Because I'm seeing this change now having not used the code before I'm questioning whether the existing behavior is actually correct. So that's why I don't agree that what's being patched is an obvious bug (yes the extra comma is wrong, my question is more focused now on whether it should be stripping DECIMAL(10,5) to DECIMAL and based on what criteria that should be happening other than existing behavior). I don't believe the existing implementation accounts for field types like this where the info in parentheses could be considered part of the type definition but was mostly designed to strip out the length values in definitions like varchar(255) or int(11). Looking at a database where I'm using FLOAT columns with Sequel Pro, I see that it considers the (10,5) part of the definition part of the length, so if that's consistent with SQL engines definitions (that is the actual type does NOT depend on a length specification) then it's good to go. And it seems like that's the case as I can run this query on a table:

ALTER TABLE `test_table` ADD `test_column` DECIMAL  NULL  DEFAULT NULL  AFTER `other_column`;

It still doesn't answer though why when types only are requested that we are doing this filtering which causes inconsistent results based on the param passed to the method; that's where my suggestion that perhaps this filtered return type should be a totally separate method.

@mbabker The usefulness of the function is a good question but what it is currently doing is incorrect. It is not meant to return a comma after the name. That and only that is what this PR is fixing nothing more and nothing less.

Perhaps a completely separate method with more options is better but that is out of scope for this PR.

@brianteeman Thanks, I will check them.

I have tested this item ✅ successfully on cf315fa

Followed a slightly different test sequence and was able to reproduce the error and confirm proper working of the patch.

I have tested this item ✅ successfully on cf315fa

The usefulness of the function is a good question but what it is currently doing is incorrect

And that's why I'm asking all the hard questions instead of just gleefully "testing this item successfully". Yes, an "obvious bug" is fixed with regard to that trailing comma, but that's the only correct thing happening.

We don't have a definition of what the filter is supposed to be other than existing behavior and we don't have a definition of why the behavior is the way it is, and that's why I'm challenging the status quo because I have a feeling that the existing behavior in general is incorrect and inconsistent.

This method has different behaviors based on a parameter and it's not clear based on that parameter that the internal API is "filtering" the type data in a way that makes the result different from the SQL engine's type definition. Is this method designed to return a SQL engine defined type or a PHP "compatible" scalar type? If it's the former, then the filter for when $typeOnly === true should be removed. If the latter, as PHP doesn't have unsigned scalars then that should be stripped as well. And the filter should be consistent regardless of whether $typeOnly is true or false. If we want a method that is returning PHP "compatible" scalar types, that should be a new API method and the existing method fixed to consistently return SQL engine types, which I would suggest is the correct interpretation of what this API method is supposed to be doing.

But as I keep saying, I'm usually ignored anyway so feel free to carry on.

@mbabker I am happy you are asking the hard questions and I don't agree you are being ignored here as we all have responded to your questions. I just don't know what to do from here on. For sure, I am not going to write a complete new method and the easy way out is to just close this PR and carry on.

The question you put forward is the same question @pjdevries and I discussed before putting in this PR. Ideally a function as one task only and modifying that behaviour based on a parameter smells to say the least. Unless we can ask the person who implemented it as to why it is done the way it is done, there is nobody who will know what it is supposed to be doing.

To clean this mess up would be to deprecate this method and write new ones that are clean and in the next major release remove this function. As I simply don't have time for this, I will close this PR and the change of this function can be done in another PR by one who has time for it.

Thank you all for participating.