Bug: table::publish() doens't update #__ucm_content

[Eighke]

A small bug, if we use publish (used by published field), it will not update the #__ucm_content. It breaks the tags on the frontend, displaying unpublished item and vice versa.

Not a big bug (we have to use tags and to use the published field) but still. :)

[nikosdion]

First, let me say this: Joomla!'s UCM is a piece of utter crap. There is no point duplicating everything just to add tags and content versioning.

That said, I think you can change the publish() method of the DataModel to check whether we have tags or versioning (both use the crappy UCM) and update the UCM content table as well.

[Eighke]

Ok, forgot about versioning. Well, It is on my todo, I'll check later.

[Eighke]

Uhm, I'm not sure if FOF3 will have the problem. We haven't anymore table. I guess I will have to test. Do you have any base component using FOF3 for testing propose?

Regarding FOF2, we haven't any easy method to know if a behavior is enabled or not. It is fine for tags (we have hasTags), but it is messy for the versioning. So I implement the behaviorParams.

https://github.com/Eighke/fof/commit/eb15c0aac1282308c760bfe3fe52cd3d7047873a

Looks good for you?

[nikosdion]

I'm not going to make any changes to FOF 2 :)

As for FOF 3, the table and model have been merged to DataModel. I believe the problem still exists, but it's easier to tackle: you only have to deal with one class, DataModel.

[Eighke]

It is not a change, it is bug fix. :D. I think we should fix all the remaning bug we found before drop it no?

Ok, for FOF3, I keep the issue open untill I can test on it.

[nikosdion]

My thinking is that this bug only affects Joomla! 3.2/3.3. FOF3 is designed to run exactly on these versions of Joomla!. Therefore the proper bugfix would be using FOF3. Don't forget that tags and content versioning wasn't really working in FOF2 anyway, at least not before two months ago.

So, in my book, the priority is fixing these issues in FOF3. Then we can discuss what to do with FOF2. Basically, does it make sense to fix them in FOF2 when the only reason it's kept with limited support is because it's included in Joomla! itself? I mean, my advice from March onwards will be "use FOF 3, you'll be better off anyway for too many reasons".

[Eighke]

Well, right. =)

I'm currently converting my testing component to FOF3. I'll check for this point after that. :)

[Eighke]

Ok conversion finished and tested. This bug is present in FOF3.

BTW, I have another problem, publish trigger the autoChecks (which is enabled by default now). So if anything wrong we cannot enabled or disabled an item anymore (the DB could have change with new version for example).

So we will have to enter in each item to fix the problems... not convenient at all (I cannot imagine if we got 100 items we want to unpublish in a row and it missing the image field~).

Not sure if you still want to keep this behavior.

[Eighke]

Oh and we still have no easy way to find if a behavior is enabled or not. Couldn't we do something like that:

public function addBehaviour($behaviour)
{
...
            $this->behavioursDispatcher->attach($observer);
            $this->enabledBehaviour[] = $behaviour;

            return $this;
...
}

We could easily use a in_array like that.

[nikosdion]

You can tell if a behavior is enabled or not with

$isEnabled = $model->getBehavioursDispatcher->hasObserverClass($className);

As for the autoChecks: when you upgrade your database schema by adding fields or changing whether an existing field can be empty you MUST also run an UPDATE command against all existing fields to ensure that your old data doesn't break. So, if you end up in a situation where you can't publish or unpublish a record because after the schema update its data has become invalid then you've screwed up big time as a developer :)

Let me take your image example. If you made the image field NOT NULL then you are relying elsewhere in your code that the image field will be non-empty. Most likely in the front-end, too. Your old records which have no image will at best result in a PHP Warning. At worst, if you also expect to manipulate (crop, fix white-balance, etc) the image in another view you will get a crash at best, a vulnerability vector at worst. This can all be avoided by using a placeholder image URL. If you are super sophisticated maybe look at the source code from Dynamic Dummy Image Generator to get an idea of how you can tackle programmatically the missing image problem.

[Eighke]

$isEnabled = $model->getBehavioursDispatcher->hasObserverClass($className);

Ok nice didn't see that. :)

For autoChecks. You cannot always update your field. For example, I have a "profile" table in my component and I added a new agencyName field. I need it to be mandatory and autoChecked. And after that my customer want to disabled all the "profile" without agencyName except the premium...

But well, my first thinking is more that I don't see the use to autocheck on publish or unpublish? We basically only update the enabled field and it is a magic field using a "magic" method.

PR soon.

[nikosdion]

And after that my customer want to disabled all the "profile" without agencyName except the premium...

Ahem, you mean something UPDATE something_something_something_table SET enabled = 0 WHERE agencyName IS NULL? Come on...

Besides, leaving the data in the database in an invalid state is bad and dangerous – not to mention daft. The NOT NULL agency field can still be an empty string. In other words UPDATE something_something_something_table SET agencyName = "" WHERE agencyName IS NULL Come on, I don't have to spell out how much is 2 + 2.

But well, my first thinking is more that I don't see the use to autocheck on publish or unpublish

What if you have to conditionally prevent enabling or disabling items? Think about a subscription. It was an activation and expiration date. Outside these dates it MUST be disabled. Inside these dates it MUST be enabled. If I allow the opposite to happen I will end up with side effects such as integrations not running, multiple and confusing emails sent to the client etc. This is a real life scenario that I'm actually experiencing, in real life. See? Nothing is as simple as it looks at first glance.

[Eighke]

No no it is not what I mean. ^^'

I mean I added a new field agencyName in the table of my generic component. This field is mandatory. My customer update his version with the last shema and will have now a new field

ALTER TABLE `#__toto_profiles` ADD `agencyName` VARCHAR(255) NOT NULL;

Then he see that now you have a new mandatory field agencyName and need to disabled all the current profiles using the administrator. Except the Premium profiles. So he will filter but "Premium" on the administrator, select All using the checkbox. Click on Unpublish. And... nope. :)

What if you have to conditionally prevent enabling or disabling items?

If you have a conditionally prevent enabling or disabling items you are not anymore in the default autoCheck but in your own custom check method, and you can do what ever you want. What I'm speaking about is only the case where you use only the autoCheck. :)

[Eighke]

PS: I was thinking about something like that. :)

$restore = $this->autoCheck;

$this->autoCheck = 0;
$this->$enabled = 0;
$this->save();

$this->autoCheck = $restore;

[nikosdion]

I am very aware of what you have in mind and I will INSIST that your schema DOES NOT reflect your data model. You are telling MySQL that the agencyName can never, ever, not in a million years be null. Yet you tell me that if the account is not premium you EXPECT that column to be null. Therefore you have screwed up your schema. The agencyName column should NOT have the 'NOT NULL' specifier. Instead, your check() method should do the following check: IF the account is premium AND agencyName empty THEN throw exception.

If you insist on the architecturally incorrect solution of conditionally disabling validation:

public function check()
{
    if ($this->getState('_disableChecks', false) return $this;

    return parent::check();
}

But I have to warn you again: if you find yourself in need of this you are most likely doing something wrong in your schema.

[Eighke]

The agencyName column should NOT have the 'NOT NULL' specifier. Instead, your check() method should do the following check: IF the account is premium AND agencyName empty THEN throw exception.

We don't speak about the exact same thing. :)

You are speaking about something that I or my component except but my schema is good. I want agencyName to be mandatory and the validation are not depending of any other fields. I except that if you create or edit a "premium" profile, the agencyName field to be mandatory. The problem is only on publish and unpublish for a user of the component. Not for myself.

Example:

• Version 1.0 : I have no agencyName field.
• My customer use this component and add 1000 profiles.
• Version 1.1 : I add a new mandatory field agencyName
• My customer see this new field and need his staff to fill the 1000 profiles. It will take time so he decides to unpublish all the profiles that are not premium and where the last update > 1 year.

Or maybe he will have some other criteria. Maybe he just need to disabled all without any exception, the time to upgrade the data.

Anyway he cannot, for any field he need to unpublish he will need to edit it and add a agencyName first.

And once again : I don't want to disabled the check(). We need it. I need to disabled the autoCheck only on publish, unpublish. Because we are currently only updating one and unique field enabled and nothing else.

[nikosdion]

I understand full well what I am saying. You still don’t get it.

IF a field is ALWAYS mandatory THEN AND ONLY THEN you make it NOT NULL in the database. In this case you don’t need to disable check() before publish/unpublish, ergo the current implementation is correct.

IF a field is SOMETIMES mandatory THEN you DO NOT make it NOT NULL in the database and ONLY enforce its presence through check(). In this case you always need to run check() before any save, therefore the current implementation is correct.

Corollary: what you are suggesting is ARCHITECTURALLY INCORRECT. Do I make myself clear? If you still don’t see why what you are suggesting is crappy database design what can I say? Read the MySQL documentation and reflect on what this "NOT NULL” thingy really means.

[Eighke]

This field is always mandatory. So it should be NOT NULL.

But, currently this field is not null, the value is an empty string.

SELECT ISNULL (`agencyName`) FROM `#__toto` WHERE `id` = 1
# Will return 0

So when I unpublish I should only change the enabled field so I'm doing :

UPDATE #__toto SET `enabled` = 0 WHERE `id` = 1

I never change the agencyName to NULL, to the check() should not send me any error, right? I have an empty string as value. I only UPDATE enabled. So, It is architecturally correct. No?

But it is just not possible to have this normal behaviour, because autoCheck work with the POST data and we need to use empty instead of isnull. That will break the publish and unpublish. :)

[nikosdion]

Indeed, BUT this will also hold true when you run save() against a non-premium profile. The architecturally correct solution is:

• add the field in fieldsSkipChecks
• specialize your check() method to enforce a non-empty agencyName if the record is premium OR set the record to an empty string if the record is non-premium and no agencyName was provided.

As you understand, if you do that then publish and unpublished are correctly covered, therefore no further change is necessary in the framework.