User tests: Successful: Unsuccessful:
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:
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.
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
@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):
$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.ContactHelperRoute::getContactRoute()
instead of doing the lookup in the plugin.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!
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.
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.
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.
Any new Feature tracker created?
@infograf768 - The tracker item is identified above as http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=31604
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.
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:
Disable author link:
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.