? Success

User tests: Successful: Unsuccessful:

avatar andrepereiradasilva
andrepereiradasilva
15 Aug 2016

Summary of Changes

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).

Testing Instructions

Apply patch all behaves normally. Business as usual.
Code review.

Documentation Changes Required

None.

Notes

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).

avatar joomla-cms-bot joomla-cms-bot - change - 15 Aug 2016
Category Administration Components Libraries
avatar andrepereiradasilva andrepereiradasilva - open - 15 Aug 2016
avatar andrepereiradasilva andrepereiradasilva - change - 15 Aug 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 15 Aug 2016
Labels Added: ?
avatar andrepereiradasilva andrepereiradasilva - edited - 15 Aug 2016
avatar andrepereiradasilva andrepereiradasilva - change - 15 Aug 2016
Title
[GsoC] JTable fields alias
[GsoC] JTable and db fields alias
avatar andrepereiradasilva andrepereiradasilva - edited - 15 Aug 2016
avatar wilsonge
wilsonge - comment - 15 Aug 2016

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

avatar andrepereiradasilva
andrepereiradasilva - comment - 16 Aug 2016

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.

avatar alikon alikon - test_item - 16 Aug 2016 - Tested successfully
avatar alikon
alikon - comment - 16 Aug 2016

I have tested this item successfully on d90c77f


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

avatar infograf768
infograf768 - comment - 17 Aug 2016

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?

@wilsonge @roland-d @rdeutz @mbabker

avatar Bakual
Bakual - comment - 18 Aug 2016

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, ...).

avatar andrepereiradasilva
andrepereiradasilva - comment - 18 Aug 2016

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.

avatar Bakual
Bakual - comment - 18 Aug 2016

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.

avatar andrepereiradasilva
andrepereiradasilva - comment - 18 Aug 2016

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.

  • For basic working com_associations needs id, title, alias and language db field. This fields are mandatory. Component item types will not work in com_associations without this db fields
  • For ACL core.edit.own (if component supports it) checking we need the created_user_id db field
  • For checkin/out checking (if component supports it) we need the checked_out and the checked_out_time db field
  • For making sure view levels are correctly supported (if component supports it) we need the access db field
  • For making sure categories are correctly supported (if component supports it) we need the catid db field
  • For better list view + filters + ordering, if component supports it, com_associations will also use ordering, menutype, level, state
avatar infograf768
infograf768 - comment - 18 Aug 2016

@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

avatar Bakual
Bakual - comment - 18 Aug 2016

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.

avatar alikon
alikon - comment - 19 Aug 2016

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.

avatar infograf768 infograf768 - test_item - 19 Aug 2016 - Tested successfully
avatar infograf768
infograf768 - comment - 19 Aug 2016

I have tested this item successfully on d90c77f

Except for contacts, all works fine.

We could make contacts a special case if necessary.


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

avatar rdeutz
rdeutz - comment - 19 Aug 2016

What is the best branch to look at too see what is changed?

avatar alikon
alikon - comment - 19 Aug 2016

staging of https://github.com/joomla-projects/gsoc16_improved-multi-lingual

thanks in advance to take care of this

avatar Bakual
Bakual - comment - 19 Aug 2016

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.

avatar infograf768
infograf768 - comment - 19 Aug 2016

@rdeutz
When testing, I suggest you apply the patch
joomla-projects/gsoc16_improved-multi-lingual#104
to that staging

avatar infograf768
infograf768 - comment - 19 Aug 2016

@bakual

instantiate it and see if the class has a specific property set (which currently isn't needed for associations).

Which property? associationsContext ?

avatar Bakual
Bakual - comment - 19 Aug 2016

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.

avatar Bakual
Bakual - comment - 19 Aug 2016

For reference, the property was introduced by JM and me with PR #7583 and #7617. It's there only since Joomla 3.4.4.

avatar infograf768
infograf768 - comment - 19 Aug 2016

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.

avatar brianteeman
brianteeman - comment - 24 Aug 2016

Setting to Needs Review


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

avatar brianteeman brianteeman - change - 24 Aug 2016
Status Pending Needs Review
avatar rdeutz
rdeutz - comment - 25 Aug 2016

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:

Parsing the models to get information about the multi language support

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.

Getting information about tables from an instance of JTable*

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:

Adding an associations helper class e.g.


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.

avatar Bakual
Bakual - comment - 25 Aug 2016

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.

avatar wilsonge
wilsonge - comment - 25 Aug 2016

Can I also just add is there any reason why we can't use the UCM associations - which should already exist?

avatar mbabker
mbabker - comment - 25 Aug 2016

Do we really want to add more code using a half-baked solution that should be yanked out of the CMS before MooTools?

avatar wilsonge
wilsonge - comment - 25 Aug 2016

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.

avatar rdeutz
rdeutz - comment - 25 Aug 2016

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

avatar Bakual
Bakual - comment - 25 Aug 2016

The content types in UCM unfortunately don't match the association content types. So using UCM isn't possible afaik.

avatar infograf768
infograf768 - comment - 26 Aug 2016

Thanks all for your input.
The project mentors and student are going to discuss this and come back to you.

avatar andrepereiradasilva
andrepereiradasilva - comment - 26 Aug 2016

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.

avatar andrepereiradasilva andrepereiradasilva - change - 26 Aug 2016
Status Needs Review Closed
Closed_Date 0000-00-00 00:00:00 2016-08-26 09:09:26
Closed_By andrepereiradasilva
avatar andrepereiradasilva andrepereiradasilva - close - 26 Aug 2016

Add a Comment

Login with GitHub to post a comment