Is there any changes required for postgres?

I couldn't test with PostgreSQL because I don't have installed on my machine, but the process of getting the type is not based on regular expression, it's based on a SQL query to its catalog, so I'm not sure if it would be affected by this issue.

I think there is an outstanding issue with finder that might be related to
this
On 15 Jul 2015 11:52, "Víctor" notifications@github.com wrote:

I couldn't test with PostgreSQL because I don't have install on my
machine, but the process of getting the type is not based on regular
expression, it's based on a SQL query to its catalog, so I'm not sure if it
would affected by this issue.

—
Reply to this email directly or view it on GitHub
#7446 (comment).

@brianteeman this: #4116? by @chrisdavenport?

I have created a SQL statement to test this PR

CREATE TABLE #__compound_types (
    `id` INT PRIMARY KEY,
    `single_float` FLOAT,
    `compound_float` FLOAT(8 , 2 ),
    `string` VARCHAR(255),
    `single_decimal` DECIMAL,
    `compound_decimal` DECIMAL(10 , 2 )
);

Once you have created this table, you need to execute this PHP statement:

$db = JFactory::getDbo();
$columns = $db->getTableColumns('#__compound_types');

$columns variable will contain this with the old regex:

{
  "id": "int",
  "single_float": "float",
  "compound_float": "float,",
  "string": "varchar",
  "single_decimal": "decimal",
  "compound_decimal": "decimal,"
}

Note: Compound types have a comma after the type

but if this PR is applied, the content will turn into this:

{
  "id": "int",
  "single_float": "float",
  "compound_float": "float",
  "string": "varchar",
  "single_decimal": "decimal",
  "compound_decimal": "decimal"
}

@zero-24 yes that is the one I was thinking of but it looks unrelated.
#failmemory
On 15 Jul 2015 13:21, "Víctor" notifications@github.com wrote:

I have created a SQL statement to test this PR

CREATE TABLE #__compound_types (
id INT PRIMARY KEY,
single_float FLOAT,
compound_float FLOAT(8 , 2 ),
string VARCHAR(255),
single_decimal DECIMAL,
compound_decimal DECIMAL(10 , 2 )
);

Once you have created this table, you need to execute this PHP statement:

$db = JFactory::getDbo();
$columns = $db->getTableColumns('#__compound_types');

$columns variable will contain this with the old regex:

{
"id": "int",
"single_float": "float",
"compound_float": "float,",
"string": "varchar",
"single_decimal": "decimal",
"compound_decimal": "decimal,"
}

Note: Compound types have a comma after the type

but if this PR is applied, the content will turn into this:

{
"id": "int",
"single_float": "float",
"compound_float": "float",
"string": "varchar",
"single_decimal": "decimal",
"compound_decimal": "decimal"
}

—
Reply to this email directly or view it on GitHub
#7446 (comment).

tested ok on mysql, mssql, and postgresql,
@vistiyos i've submitted the relative postgresql driver fix pr on your repo

@alikon thank you for your PR. I don't really know PostreSQL that much, that's why I didn't provide that fix for that DBMS

Anybody else have test this PR?

Does what it says but it also effects the enum type. When you take akeeba admintools for example and look at the enum type variable "status" in "#__admintools_scans" before applying your patch I get

enum'run','fail','complete'

after

enum'run''fail''complete'

I have not analysed if there are any negative repercussions. Maybe @nikosdion can help.

The commas in the enum type are mandatory, otherwise anything using enums will fail miserably. And not just that. The current expected behaviour is to get the full details of a column's type, not the butchered version.

-1 (NO GO) from me. This patch breaks b/c in a major way and is wrong in principle.

Suppressing commas is definitely NOT the right way to parse field types with more than one parameters. Your code which consumes the results from getTableColumns should be aware of the kind of results you are going to receive from the DB server and proceed accordingly. In fact, it would make a world of difference not knowing the decimal's accuracy (significant digits) when using the Joomla!-provided API. In the end of the day it's just not prudent butchering Joomla! when you can't fix the bugs in your own code.

@nikosdion So according to your comment, there is a bug on Joomla already, and it's removing the length for types, am I right? If everybody agree that is a bug, I would suggest to remove that regular expression from the code.

BTW: I provided this PR just to contribute to Joomla, I don't contribute for my own interests or my company ones. Apologize me if I have understood the other way around.

No, there is NOT a bug in Joomla!. For example, "enum" means nothing without the possible values. "float" is also meaningless without the precision. If you do not get this extra data you can neither reproduce the field type nor detect its possible changes. Since this method is used to detect if schema changes are required Joomla! is right in returning the values it is currently returning.

Is the format of the data returned problematic? Yes, it is. However changing that is not possible in Joomla! 3 because that would break backwards compatibility and that would be a bug. I know it is counter-intuitive but since we follow semantic versioning it is what it is.

What about Joomla! 4? Right now the Architecture Working Group is examining if we can use a database abstraction layer (namely, Doctrine DBAL) to overcome this and many more issues. If this works out then the point if this PR is moot. If it doesn't work out we can revisit the data format returned by this method.

I hope that this background info helps you understand better the context of my reply :)

@nikosdion Thank you for trying to explain better your comment. Looking forward to see if Joomla 4 includes an agnostic database layer such as Doctrine (I have worked with it on Symfony and it's just awesome).

We're trying hard to make that happen. It is a difficult task (trust me, you don't want to know the amount of changes that have to go in to just redo the event system alone...) but we're resolved to deliver something good and in a reasonable amount of time :)

@vistiyos

Considering the feedback I am going to close this PR. Thank you for your contribution.