Success

User tests: Successful: Unsuccessful:

avatar AmyStephen
AmyStephen
31 Jul 2013

ALL INSTALL CODE READY. THIS IS READY FOR TESTING AND USE.

Moves the contact_table out of the Articles Model into a new plugin that fires on Content Prepare. The link and the contact id for the articles are determined in that plugin.

Benefits of this approach:

  1. Content will never again be dropped when the article author's user id is not assigned to a contact. This has been a recurring problem.
  2. Content will never be "duplicated" when the article author's user id is assigned to multiple contacts. This has been another recurring problem.
  3. The same support for language filtering exists with this approach as is used today.
  4. Avoids the newest problem introduced due to the increasing complexity of the SQL for SQL Server.
  5. Prevents the additional query load incurred for those not using the feature (including any normal execution of the blog layout which never has included this link.)
  6. Query slow down due to improper use of GROUP BY statements will not ever be re-introduced, even for only a few database types. GROUP BY is not a tool for fixing improper joins. (And, in fact, Mark Dexter found that it didn't fix the multiplicity issue.)
  7. The feature to allow a contact to serve as the source of an author profile continues to be offered in a BC manner.
  8. The feature is implemented in a flexible manner. Developers can offer their own author profile simply by turning off the core plugin and implementing their own which links to their author profile feature.
avatar AmyStephen AmyStephen - open - 31 Jul 2013
avatar AmyStephen
AmyStephen - comment - 31 Jul 2013

OK, that should be it.

If this is accepted, would someone please update the install SQL to add two plugins to the install? Both should be enabled on install. I'm not sure how to do that anymore - so, I'm not going to attempt it in this patch.

Test using an Articles List.

Enable the author link option:

  • No contact associated with author (no dropped articles)
  • A single contact associated with an author (link to the contact page is correct)
  • Multiple contacts associated with author (no duplicated articles - each link is correct)
  • Verify language filtering for each of the above

Disable author link:

  • Plugin queries do not run (those who do not use the author link are not penalized with the query)

Also - I understand the Bug Squad has a set of tests for this, already. Those tests should be used, too.

The problem reported on the tracker 31604 is fixed, too.

avatar Bakual
Bakual - comment - 31 Jul 2013

Personally I think the tags should go either into the view or the layout. The code would be easy enough to be handled in the layout, however due to B/C issues it probably needs to be in the view I think.

I like the contact plugin as a solution for the current issue. However I would like to see it extended a bit. Instead of just returning the contactid, it could create the URL pointing to the contact. The layout could then directly use this link instead of the contactid. For B/C we can keep the contactid so no templates will be broken.

This way there could be different plugins, pointing to different extensions. The user could choose that the link doesn't point to the core contact page, but instead to the Kunena profile page or to a page from Community Builder, JoomSocial or whatever extensions there are.
Of course this would only work on layouts which support the new way, older layouts would always link to the contact.

avatar AmyStephen
AmyStephen - comment - 31 Jul 2013

Removed the tags plugin - just to keep the patch focused on this Contact problem that has been plaguing Joomla users for three years.

Agree @Bakual on the benefits of moving the link determination into the plugin and then using that link in the layouts. As you have correctly stated, by retaining the contactid value in the resultset (as this patch does), BC is still maintained and the entire feature can now be replaced by any developer with another Author Profile.

This is 100% BC.
It fixes the long term problems with the Articles Model.
It not only continues to provide the feature -- but it does so in a more flexible manner.

Please, consider this patch.

Note: language strings are introduced in this patch

avatar Bakual
Bakual - comment - 31 Jul 2013

@AmyStephen Very nice work. I did some tests and couldn't generate any issues so far.
The only drawback I see is that the "link_author" parameter doesn't do anything now if the plugin is disabled.

Also I had to discover and publish the plugin first. This needs to go into the database install and update SQL files so the plugin is installed and published by default. Otherwise we get B/C issues.

Still some suggestions (hope you don't mind):

  • Expand the plugin description a bit. "Enable contacts for content." is a bit short. Something like "Provides links between content author and contact page". I'm not an english native speaker, so it's probably not best spelled :-)
  • I don't like the part with $query->select('1 as filter_language');. Putting something in a query whose sole purpose is that a plugin can detect a state seems wrong. Since this particular state is only set by JLanguageMultilang::isEnabled(), this can also be used in the plugin to determine if the language filter should be applied or not. It would also make the query a bit more self-explaining as well.
  • Just thinking loud: To get the proper link, we could maybe also use ContactHelperRoute::getContactRoute() instead of doing the lookup in the plugin.
avatar AmyStephen
AmyStephen - comment - 31 Jul 2013

On Wed, Jul 31, 2013 at 2:29 PM, Thomas Hunziker
notifications@github.comwrote:

@AmyStephen https://github.com/AmyStephen Very nice work. I did some
tests and couldn't generate any issues so far.

Excellent!

The only drawback I see is that the "link_author" parameter doesn't do
anything now if the plugin is disabled.

True, but isn't that how it should be?

Also I had to discover and publish the plugin first. This needs to go into
the database install and update SQL files so the plugin is installed and
published by default. Otherwise we get B/C issues.

Above #1623 (comment),
I requested that help:

If this is accepted, would someone please update the install SQL to add two

plugins to the install? Both should be enabled on install. I'm not sure how
to do that anymore - so, I'm not going to attempt it in this patch.

Just don't know how.

Still some suggestions (hope you don't mind):

Really appreciate your involvement, Thomas.

  • Expand the plugin description a bit. "Enable contacts for content." is a bit short. Something like "Provides links between content author and contact page". I'm not an english native speaker, so it's probably not best spelled :-)

Good point! How about this? (add a tiny bit).
*
Provides link between content author and contact item for use as an Author
Profile.*

  • I don't like the part with $query->select('1 as filter_language');. Putting something in a query whose sole purpose is that a plugin can detect a state seems wrong. Since this particular state is only set by JLanguageMultilang::isEnabled(), this can also be used in the plugin to determine if the language filter should be applied or not. It would also make the query a bit more self-explaining as well.

Agree. Your approach is much better, my approach was definitely hackish -
just wanted to ensure I was using exactly the same criteria. I had no idea
what to grab from the API.

Change made according to your direction. Thanks!

  • Just thinking loud: To get the proper link, we could maybe also use ContactHelperRoute::getContactRoute() instead of doing the lookup in the plugin.

Hm. On that idea, I think I disagree.

First, I got that code directly from the layouts, so, it should produce the
same results as the layouts. For me, especially with URLs, that's always my
primary concern.

Second, the signature of getContactRoute requires catid and
language(optional), along with the contact id value. So, don't have catid.

IMO, the safe route is to do what the layouts did, which is the code in
place.

If you feel strongly on this and have ideas on how to get that missing
signature data, glad to consider it. Otherwise, I think we're okay on the
link. It's using the same approach as is in place now. Always a good
defense. ;-)


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

Thanks so much, Thomas. Should be there now, let me know what you think!

avatar Bakual
Bakual - comment - 31 Jul 2013

The only drawback I see is that the "link_author" parameter doesn't do
anything now if the plugin is disabled.

True, but isn't that how it should be?

Yeah, it is. Just wanted to note it in case questions arise.

Above #1623 (comment), I requested that help:

I can have a look at the needed SQL changes for the plugin install. I didn't remember your call. But it will take some time as tomorrow is national holiday here. So if someone else is faster I don't mind :-)

Provides link between content author and contact item for use as an Author
Profile.

Much better!

First, I got that code directly from the layouts, so, it should produce the
same results as the layouts. For me, especially with URLs, that's always my
primary concern.

Good point. I would have probably done the same.

avatar AmyStephen
AmyStephen - comment - 1 Aug 2013

OK - this one is ready. Installation script for the new plug, model changed for articles and article, all article layouts changed so that contact_link is used instead of contact_id. (Contact_id is still available - fully BC.)

Gave it quite a bit of testing -- should be good to go for 3.X.

avatar AmyStephen
AmyStephen - comment - 1 Aug 2013

2.5 fix is #1627

Both are ready for testing. I have tested both extensively and believe this is good to go. Install code is ready for 3.x. 2.5 is a very simple fix, no plugin, no install changes or new features.

These patches will finally fix the contact join problem - it will not recur.

avatar infograf768
infograf768 - comment - 1 Aug 2013

Any new Feature tracker created?

avatar Bakual
Bakual - comment - 5 Aug 2013

Sorry for the delay.
I tested with a fresh installation and the plugin is installed, published and everything works fine for me. So that one should be good.
However I think for the updates, there is the SQL statements missing to update the database with the new plugin. You need to paste the SQL command into the file /administrator/components/com_admin/sql/updates/mysql/3.1.6.sql (and the respective files for sqlazure and postgres). Similar to how the "IDNA Convert" library was done in the 3.1.4 file.

avatar AmyStephen
AmyStephen - comment - 30 Nov 2013

@Bakual - I'm going to close this now. Thanks for moving it forward.

Add a Comment

Login with GitHub to post a comment