User tests: Successful: Unsuccessful:
In GsoC multilingual project we need to know the names of the db fields of the components item types.
The problem is the core (and 3º party) uses different names for the db fields in each components item types.
Fortunally there is already a _columnAlias
JTable property that is not being used and can be used for that mapping.
So this PR uses that property in JTable for components in the core that use associations (com_contact, com_content, com_categories, com_newsfeeds and com_menus).
Apply patch all behaves normally. Business as usual.
Code review.
None.
In the project is used here: https://github.com/joomla-projects/gsoc16_improved-multi-lingual/blob/staging/administrator/components/com_associations/helpers/associations.php#L121-L143 to get the component item type db fields.
For chosing which should be the primary field names, i followed the same logic as the UCM_content table fields (https://github.com/joomla/joomla-cms/blob/staging/installation/sql/mysql/joomla.sql#L1677).
Category | ⇒ | Administration Components Libraries |
Status | New | ⇒ | Pending |
Labels |
Added:
?
|
Title |
|
Didn't notice that.
BTW in the core i think it's only also used in com_banners (https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_banners/tables/banner.php#L36)
As base for normalizing this i used the #__ucm_content
table field names: https://github.com/joomla/joomla-cms/blob/staging/installation/sql/mysql/joomla.sql#L1677.
Since there is no published
in that table (that table as alreasdy the correct db field name state
) i don't see the need to add the alias for published
, but for B/C is better to not touch it IMHO.
Anyway, please check if everything is fine with this PR.
I have tested this item
While testing, we found an issue for contacts:
The db, alas, contains a "state" column (for the adresses) which has nothing to do with published
.
Therefore we would need to add 'state' => 'published',
to
protected $_columnAlias = array(
'title' => 'name',
'created_time' => 'created',
'created_user_id' => 'created_by',
'modified_time' => 'modified',
'modified_user_id' => 'modified_by',
);
My tests here show this has no impact on filtering contacts by state or displaying the correct field value for "state".
Could it have an impact on 3rd party extensions? I.e. would it be enough B/C?
Imho, it sounds like an architectural issue if 3rd party extension need to define alias mapping so another extension (this multilang stuff) can work properly. But I don't see yet why you need that to begin with. It should work with any db fields anyway, not only with the "standard" ones found in core. 3rd parties will have all kind of field names, some may be matched to "core" ones and some don't. Some will also miss "core" fields (eg created, modified, ...).
Imho, it sounds like an architectural issue if 3rd party extension need to define alias mapping so another extension (this multilang stuff) can work properly. But I don't see yet why you need that to begin with.
We did our best to solve this problem.
The current code for using this mapping is here https://github.com/joomla-projects/gsoc16_improved-multi-lingual/blob/staging/administrator/components/com_associations/helpers/associations.php#L120-L143
From your comment i understand you have a better solution to solve this problem, so please say so.
It should work with any db fields anyway, not only with the "standard" ones found in core. 3rd parties will have all kind of field names, some may be matched to "core" ones and some don't. Some will also miss "core" fields (eg created, modified, ...).
And it works like that, as long as the db fields mapping are defined like this PR.
From your comment i understand you have a better solution to solve this problem, so please say so.
No, I have no clue. I don't even know what that GSoC project tries to achieve, why it needs to know the db fields of 3rd party extension and why it needs to map (a subset of?) them. So it's hard to judge if there would be a better approach.
Maybe such a alias mapping is indeed the best approach, but it looks a bit fishy from an architectural point of view. Other extensions should never have to know anything about internals of 3rd party extensions. There should be an API the extension can consume. But then as said I don't know what you're going to do with that mapping data.
No, I have no clue. I don't even know what that GSoC project tries to achieve, why it needs to know the db fields of 3rd party extension and why it needs to map (a subset of?) them. So it's hard to judge if there would be a better approach.
id
, title
, alias
and language
db field. This fields are mandatory. Component item types will not work in com_associations without this db fieldscreated_user_id
db fieldchecked_out
and the checked_out_time
db fieldaccess
db fieldcatid
db fieldordering
, menutype
, level
, state
@Bakual
I suggest you test the GSOC new component com_associations.
It lets take care of multingual associations in a central place and not item per item in their manager as we have now. It is an important feature which has been missing in Joomla since the first implementation of associations.
Its purpose is to be universal, i.e. if some basic requirements are met, then any component can become multingual and be treated in com_associations.
Some extensions, including your sermonspeaker one, need some corrections to be eligible.
New extensions will need to follow a special doc.
We face here a problem which is already taken care of in core by the implementation of $_columnAlias for some core components as even core is not following basic simple rules to name the columns in various tables.
You can help, and we accept any help and suggestion.
Please. again, install and test this branch:
https://github.com/joomla-projects/gsoc16_improved-multi-lingual
That's what I mean with the architectural flaw.
Your component will apparently directly access the database tables of other (3rd party) extensions, which it should not from an architectural point of view.
It should consume an API to get its data. It shouldn't check the ACL or if it's checked out for an item of a foreign extension. That will likely produce issues later.
It either has to use a plugin event like the search components do or an API endpoint in the components table or model classes. Eg for checking if the user is allowed to edit it has to call the "allowEdit" method in the extensions model.
If you're going to try to read extensions tables yourself, you will fail sooner or later and extension developers will likely hate you. It will be another feature that ends up on the list "Refactor for 4.0".
Or I completely misunderstand something fundamental here.
We (GSoC multilingual project) are not introducing new "architectural flaw" IMHO
https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/table/table.php#L1675
we are just using something that is already in core since 3.4 and is still not deprecated IIRC,
If this new feature will be added on the list "Refactor for 4.0" is quite normal from my personal POV,
can you tell us what CMS features are not going to be added to that list ?
One of the main goal of this project was to be ready for 3.7.0 not for 4.0.
That said, we really appreciate your constructive critics, and we will be very happy with your help to improve/eliminate the "architectural flaw" in the next future, but at this point of the GSoC project (we have something less than a week) this is not manageable.
I have tested this item
Except for contacts, all works fine.
We could make contacts a special case if necessary.
What is the best branch to look at too see what is changed?
staging of https://github.com/joomla-projects/gsoc16_improved-multi-lingual
thanks in advance to take care of this
but at this point of the GSoC project (we have something less than a week) this is not manageable.
Which doesn't mean it has to be merged as is.
Just having glanced over it to see what you do here I saw another thing I didn't like much: You're populating the association content types by looking up all models from (almost) all extension, instantiate it and see if the class has a specific property set (which currently isn't needed for associations).
Personally I would rather have a helper method (or a plugin) in each extension which provides the data/API you need.
@rdeutz
When testing, I suggest you apply the patch
joomla-projects/gsoc16_improved-multi-lingual#104
to that staging
Which property? associationsContext ?
Yes. It's an existing property that isn't strictly needed for associations to work. 3rd party extensions may use it or not.
it is not strictly needed indeed, but now should be for the extension to be considered automatically in com_associations. As well as some other stuff (for example a correct save2copy with generatenewtitle(). A specific tutorial will explain all the basic requirements and will be available to 3rd party devs.
Setting to Needs Review
Status | Pending | ⇒ | Needs Review |
I have looked at the Project and the implementation. I like how easy it is to copy an article and make associations I think that’s well done. Unfortunately I have to agree with @Bakual when it comes to the implementation. I will first list the problems I see and then make suggestions how we can make if different (maybe better), so here the problems I see:
I fact that is guessing, you look at properties and if they are set you say it’s supports associations. But what is when I use this properties in my component for something different then your guess is wrong.
Parsing components models comes with the danger that a model can produce a fatal error so the associations component will end with a fatal error. Guess who will be blamed for it.
Most likely the same as for the models.
There is also a possibility that a JTable* object is not stored in administrator/COMPONENT/tables then you will not find it. Not so bad but also not nice
To keep a long story short I think it is the wrong way. Here is what I would do:
in administrator/components/com_contacts/helpers/associations.php
class ContactsAssociationsHelper extends AssociationsHelper {
}
So you can find out if associations are supported only by checking if the file exists. I have seen there is an old association.php helper file, I wouldn’t recycle it because it serves a different purpose.
AssociationsHelper should implement an interface with following methods:
hasAssociationsSupport()
@return boolean
getItemsTypes()
@return array The itemtypes
getTableFields()
@return array The table fields associations
Plus something for ordering, you might can skip the AssociationsHelper base class and implement the interface directly.
That will make the whole parsing components part obsolete and give the power in the hands of the extension developer how and what he supports.
I have also seen some other minor things with a mix of hardcoded HTML and a layout but that is not such a big deal.
I don’t think that is too hard to implement/change.
Either use the approach Robert outlined or you can try to work with plugin events. Use an own group for those plugins and you can use plugin events like getSupportedExtensions
which return the array with the fields. and maybe even instructions how to fetch the list or item data. Or you can do that with separate plugin events. It could even work using com_ajax.
Can I also just add is there any reason why we can't use the UCM associations - which should already exist?
Do we really want to add more code using a half-baked solution that should be yanked out of the CMS before MooTools?
We won't be removing UCM in the near future - certainly not in Joomla 4. Mootools however will be removed in Joomla 4. So yes.
I looked for something that is not such a big change, what I proposed is more or less moving and removing code. Not a big fan of UCM to be honest
The content types in UCM unfortunately don't match the association content types. So using UCM isn't possible afaik.
Thanks all for your input.
The project mentors and student are going to discuss this and come back to you.
it' clearly the project will not be ready soon so there's no point in keeping this PR open.
I created an issue in the project github to discuss this joomla-projects/gsoc16_improved-multi-lingual#114
Please discuss there.
Status | Needs Review | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-08-26 09:09:26 |
Closed_By | ⇒ | andrepereiradasilva |
We are using that property in several components - even in one of the tables you've modified :P https://github.com/andrepereiradasilva/joomla-cms/blob/d90c77fd3348eea11a640683acdf838b3a9d1b6d/libraries/legacy/table/content.php#L50