Allow NULL for #__content columns

[gunjanpatel]

• Allow null value for, #__content table images, urls, attribs and metadata columns.
• Please follow testing guidelines from here Cannot add article in Joomla 3.2 with PostgreSQL #3087

[Bakual]

Copying issue text:

---

Joomla Version: 3.2.2
PostgreSQL version: 9.1+
OS: Ubuntu 12.04 and 13.10
PHP version: 5.3.10 (ubuntu 12.04) and 5.5.3 (Ubuntu 13.10)

Error
Save failed with the following error: Database query failed (error # %s): %s SQL=INSERT INTO "rvyni_content" ("id","title","alias","introtext","fulltext","state","catid","created","created_by","created_by_alias","modified","publish_up","publish_down","images","urls","attribs","version","metakey","metadesc","access","metadata","featured","language","xreference") VALUES (0,'Test Title','test-title','

Test Description

','',1,2,'2014-02-11 14:57:35',509,'','1970-01-01 00:00:00','2014-02-11 14:57:35','1970-01-01 00:00:00','{"image_intro":"","float_intro":"","image_intro_alt":"","image_intro_caption":"","image_fulltext":"","float_fulltext":"","image_fulltext_alt":"","image_fulltext_caption":""}','{"urla":false,"urlatext":"","targeta":"","urlb":false,"urlbtext":"","targetb":"","urlc":false,"urlctext":"","targetc":""}','{"show_title":"","link_titles":"","show_tags":"","show_intro":"","info_block_position":"","show_category":"","link_category":"","show_parent_category":"","link_parent_category":"","show_author":"","link_author":"","show_create_date":"","show_modify_date":"","show_publish_date":"","show_item_navigation":"","show_icons":"","show_print_icon":"","show_email_icon":"","show_vote":"","show_hits":"","show_noauth":"","urls_position":"","alternative_readmore":"","article_layout":"","show_publishing_options":"","show_article_options":"","show_urls_images_backend":"","show_urls_images_frontend":""}',1,'','',1,'{"robots":"","author":"","rights":"","xreference":""}',0,'*','') RETURNING id

[wilsonge]

Is there a reason why this can be null? As I recall all these columns are json encoded which means that on save at minimum they should have a {} tag in them to reflect they are empty.

[lunguyenhat]

@test - It's working :)

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4687.

[alikon]

@test success
@wilsonge from data design POV you should ask :'every article item have an image?' I think the answer is no

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4687.

[brianteeman]

Multiple good tests thanks - setting to RTC

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4687.

[wilsonge]

Of course every item won't. But because it's json_encoded - even a null value means that the actual value stored in the database will NOT be null. See example 2 on the PHP documentation with an empty array http://php.net/manual/en/function.json-encode.php

$b = array();

echo "Empty array output as array: ", json_encode($b), "\n";

giving the output

Empty array output as array: []

i.e. there's an open close bracket tag

The data that Thomas has pasted in above from the original issue shows this.

{"image_intro":"","float_intro":"","image_intro_alt":"","image_intro_caption":"","image_fulltext":"","float_fulltext":"","image_fulltext_alt":"","image_fulltext_caption":""}

is what should be being inserted into the images column in this case (even tho all the rows are empty)

ditto for the URLs

{"urla":false,"urlatext":"","targeta":"","urlb":false,"urlbtext":"","targetb":"","urlc":false,"urlctext":"","targetc":""}

So setting that it should not be null is not the correct solution to this issue - even if it does provide a temporary solution

[wilsonge]

I'm unsetting this from RTC and setting to Needs Review for a bit because I want someone else to look over this. Something is very wrong here for the reasons I described above - and I don't want us to find in 6 months time this masked some horrendous bigger problem

[gunjanpatel]

Thanks @wilsonge Valid point. Needs review. Thanks.

[phproberto]

I agree with @wilsonge

This is not the right fix.

[alikon]

Just not consider only the code perspective,but think also in term of SQL constraints, surely there is a bug on the code side for having a null value on these fields, but the nature of that kind of fields I think is nullable. I agree too more reviews needed

[Bakual]

Just not consider only the code perspective,but think also in term of SQL constraints, surely there is a bug on the code side for having a null value on these fields, but the nature of that kind of fields I think is nullable. I agree too more reviews needed

According to Georges argument, the field never should have a null value and thus indeed shouldn't be nullable. Instead it should be an empty JSON string {} and this also should then actually be the default value.

[wilsonge]

Exactly! The fact we have more than just an empty string in the original issue means that something is significantly more wrong.

[gunjanpatel]

Actually, we will have to change type from TEXT to VARCHAR.
For example:

ALTER TABLE `#__content` CHANGE `images` `images` VARCHAR( 5120 ) NOT NULL DEFAULT '{}'

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4687.

[wilsonge]

Presumably as people use this on Mysql without any issues day in day out that would be for postgres only tho?

[gunjanpatel]

Yup, this is for example. I will remove alter queries from mysql file, actually whole mysql file and will change postgres alter query with help of @alikon :)

[gunjanpatel]

I just revert MySQL changes. :)

[alikon]

should be
ALTER TABLE #__content ALTER COLUMN images SET DEFAULT '{}';
ALTER TABLE #__content ALTER COLUMN urls SET DEFAULT '{}';
ALTER TABLE #__content ALTER COLUMN attribs SET DEFAULT '{}';
ALTER TABLE #__content ALTER COLUMN metadata SET DEFAULT '{}';

[wilsonge]

you should use quotes for the names "#__content", "images" etc.

[wilsonge]

Guys even so the bigger question is that in the bug report we can see there is data that should be being written to those columns (I highlighted it in my big explanation post earlier) so why does postgres seem to think that big json expression is equal to null?

[alikon]

this issue arise only on frontend content submission on backend it works as for #3087
so i think is dued by some malformed json expression triggered from "frontend code"
postgresql works well with json as i know, mysql is a lot more permissive than postgresql on SQL standards
and this is a clear example
postgesql can helps us to trap this kind of bugs
;)

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4687.

[wilsonge]

So in that case can we close this PR all together and fix the malformed json?

[wilsonge]

INSERT INTO "q8dzo_content" ("title","alias","introtext","fulltext","state","catid","created","created_by","created_by_alias","publish_up","publish_down","images","urls","version","metakey","metadesc","access","featured","language") VALUES ('Foobar','balh','<p>asdfadsfasdf</p>','',1,14,'2014-10-19 16:09:48',371,'','2014-10-19 16:09:48','1970-01-01 00:00:00','{"image_intro":"","image_intro_alt":"","image_intro_caption":"","float_intro":"","image_fulltext":"","image_fulltext_alt":"","image_fulltext_caption":"","float_fulltext":""}','{"urla":false,"urlatext":"","targeta":"","urlb":false,"urlbtext":"","targetb":"","urlc":false,"urlctext":"","targetc":""}',1,'','',1,0,'*') RETURNING id

OK So first bug is that when images and urls are turned off we have no images or url fields in the frontend hence why urls and images will throw an error. I guess we need to form a empty blank json variable in that scenario (imo we should do that in the table class tho - as despite it works on mysql it doesn't make it 'right').

With that parameter turned on to show the url and images fields I get the query error above. Note that urls and images appear to have valid json. So I think we can rule out those from our issue list. So now working on spot the syntax error :P

[Bakual]

We can actually do both ways. If you set the correct default value ({}) in the database like proposed in this PR, you can save a few lines in the code. However it still could make sense to also make sure the code always sets the value.

[gunjanpatel]

@Bakual I have updated code and set {} as default for postgres.
Thanks to @alikon and @wilsonge

[Bakual]

After reading a bit about this stuff, I came to the conclusion that it's better to allow NULL values for those fields.
The reason is that MySQL doesn't allow to set a default value for TEXT fields. While it would work for PostgreSQL and MS SQL to set a default value of {}, it would fail for MySQL.

I'd rather have it consistent across the databases and thus allow NULL in all three supported databases. For our code it shouldn't matter anyway.

What do you think?

[gunjanpatel]

I think it's a good idea. 👍 If we agree on it then I will roll back gunjanpatel@170274b changes.

[Bakual]

I would prefer it being nullable.

I was tinkering a bit yesterday with that: https://github.com/Bakual/joomla-cms/compare/SqlTextFieldsNullable which would make all text fields nullable in MySQL and PostgreSQL.
I haven't had time yet to test it so didn't create a PR yet.

[wilsonge]

I really still feel like in the majority of cases it is just poor php coding by us rather than cases where these fields should be nullable.

[Bakual]

I really still feel like in the majority of cases it is just poor php coding by us rather than cases where these fields should be nullable.

It's both. Poor database design and poor coding 😄
From a database design perspective, text fields are either null or they have to contain a value. Imho there is no point in storing an empty string. It only consumes memory without adding value.

The better question is: Why should the field not be allowed to have NULL? It makes sense if you define a default value, then the field is (obviously) not going to contain NULL. But if there is no default value, then it should actually be NULL if not defined by code.

[wilsonge]

Well theoretically it should be more than just a empty json string. It should contain all the default values of the parameters we are not showing!

[sovainfo]

It is not considered bad database design! In general it is not adviced to allow null for any column. At the minimum avoid null for PK, FK, numeric columns and colums involved in query selection criterea.
When allowed make sure your code deals with null values as expected. A column with a domain of null, a, b, c; a query with condition 'column not equal b' doesn't return null rows. With a domain of '', a, b, c; rows with '' are included in the result as expected by many. Ofcourse 'column is null or column not equal b' would be the correct condition.

Don't know enough about JSON, but don't like storing empty values there either. There is probably a good reason to waste so much space!

[Bakual]

It is not considered bad database design! In general it is not adviced to allow null for any column. At the minimum avoid null for PK, FK, numeric columns and colums involved in query selection criterea

I agree with that. If possible don't allow null and provide a default value.
Here we're only talking about text (and mediumtext) fields. They have the issue that in MySQL, you can't define a default value for text fields and it uses an empty string instead of null if null isn't allowed. In PostgreSQL it doesn't do that and you'd have to define a default value or make sure the code always passes a value (which it currently doesn't).
So we either can define a default value in PostgreSQL which would make the schemas different from MySQL, or we can allow null which would work for all databases.

The typical fields it possibly affects are:

• description
• params
• metakey
• metadata
• metadesc
• urls
• images

I don't think those are used anywhere as keys or selection criterias.

In com_content there are two additional text fields introtext and fulltext. One could argue what to do with those. Imho we could leave them as I'm quite convident the code makes sure those are set always and it actually would make sense that the query fails if those are not set.

[sovainfo]

No argument there. Would like Joomla to use the at minimum rule for the database. Let the model decide to respect null values or not.

[brianteeman]

Where are we with this one? @Bakual @sovainfo @wilsonge

This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/4687.

[sovainfo]

Looks like it is @Bakual @sovainfo against @wilsonge @phproberto as in dropping not null constraint vs keeping it.
No solution has been provided for keeping it and dropping it is rejected. Doesn't that normally mean it is closed in 4 weeks time if no solutions is provided?

[Bakual]

I've pinged PLT to have a look.
Basically there are three ways to solve it:

1. Define a default value -> isn't possible in MySQL since it doesn't allow to set a default value for text fields. But would work for the other DBs. It would however make the schemas different.
2. Allow NULL to be stored. Should work in all DBs. However I don't know if there was a reason to not allow NULL for those fields.
3. Make sure in the code that there is always a value stored. This would work for all DBs obviously but is likely to break sooner or later again and is a lot of work to find all instances.

This PR uses approach 1.
I did a branch to see how it could be done for approach 2 (Bakual/joomla-cms@joomla:staging...Bakual:SqlTextFieldsNullable) and George did some PRs with approach 3.

[wilsonge]

Having had some time to think about it my argument is that actually to stop null columns being null etc. is exactly the point of the JTable::check() method. This is probably a symptom that we need to add in much more rigorous checks there for the data we are storing into the database.

I'm gonna try and get a PR in soon with an example for that in com_content that will also fix the frontend editing there (it's not fixed in 3.4 alpha yet)

[wilsonge]

Whipped this up before I go to bed #5230 (note haven't tested it but will do tomorrow) but gives you an idea of how I see we should go

[alikon]

the basic check #5230 does his job well, is simple, only in a logical place adn fix the #3087 issue
and could be enough for let 3.4.0 run better on postgresql
so, is a temporary fix we always need to clearly define what exctaly is the nature of these kind of fields, and how to manage, but i think this could take more time...

[Bakual]

I still wonder why those text fields are defined as NOT NULL to begin with. Nobody seems to know...

[wilsonge]

Well because they shouldn't ever be null. They should have a full json string. Even my thing is a bit of a hacky way around it - really we should pick out all the defaults and store them

[Bakual]

NULL basically means undefined, which would be exactly the case if the form doesn't define it.
Our code would threat it the same way as if it is empty.
I have a feeling that we make our life harder than needed by not allowing NULL for those fields.

we should pick out all the defaults and store them

We don't have any default values there. It's all empty by default 😉

[sovainfo]

@wilsonge What makes you say they should never be null. What part of the application requires them to have values. Only when the application requires it the NOT NULL is justified. When implementing the minimum rule as mentioned above these would not be justified. So far all applications can cope with these fields having no value entered yet. That makes the NOT NULL unjustified.

[wilsonge]

No I disagree. These are the parameters for the articles. They should always have values. We then have emergency fall backs in the article when we add default values when we retrieve the parameter values (stored in the attribs) doing stuff like $param->get('foobar', 'default');. If we could (and in an ideal world we should) expect the value to always be defined then we wouldn't need this. The reason it's not always set (of course this is slightly biased because sometimes there are new params etc.) is because when the database has null values for the fields because we're not storing anything there.

Ditto for images. If we aren't showing images then we should have this stored in the database not just guessed at when the entire images column is empty

We should only need to define the fallbacks once. And that place is in the form.xml - hence why I stated above my thing is a hack because it doesn't actually solve that problem - although it solves the immediate postgres issue.

[Bakual]

We don't have any default values for images, urls, metadata and attribs. All parameters are saved with an empty value, if anything is stored at all.
The default values are not set in the form.xml (and should not set there!). They are set in the component options or there are (rightfully) no default values at all.

It absolutely makes no difference if the value in the database is NULL, an empty string or a JSON string with parameters defined with empty values.

[sovainfo]

First of all, I object to these attributes of an article being called parameters.
Second, the question was what makes you say they should have values! Just repeating the statement doesn't explain why you say it and doesn't make your statement true!

For example: images: The business logic of the application doesn't require an article to have images. When no images are provided the application functions just as well. Compare this to the username and email of the user. The business logic within Joomla requires these to be present. A user is considered invalid without them. Those attributes should have NOT NULL. Which makes them required to be entered upon creation of a user. It is not allowed to provide them later. Compare that to images: they can be specified at any time, added or removed. An article is considered valid with or without them. That makes them OPTIONAL, the attribute should allow NULL, respecting the minimum rule!

[Bakual]

Merged this PR as is into staging. It will fix the issue at hand even if it's kind of a workaround 😉
Thanks for all the work, discussing and testing!