Thankyou for taking over this :) Something I've been meaning to push for a while!!

Updated to master

@Bakual can I be a pain and ask that you update this to master so it can be tested and hopefully merged.

@betweenbrain Saw it just the second after I posted on Twitter
Unfortunately the conflicts are coming from some changed SQL files... I'll dig into it this evening.

@Bakual Thanks! Let's this one in, it's way too important to gather digital dust.

Updated and moved the update sql stuff to new SQL files.

Posted successful test on tracker. Tested this afternoon and works like a charm
You may blame the J!Tracker Application for transmitting this comment.

I found two issues why the featured view didn't work.

• One in the plugin because it checks for a specific context and the featured was missing.
• The other is in the featured view itself, where the non-existant $this->params was passed to the onContentPrepare event. It should be $item->params instead. That's actually an unrelated bug

I'll update the PR with those changes. Thanks for the good testing!

Remind me to buy you a beer. The code looks good, I will test this this afternoon.

This is great Thomas! I would like to test this afternoon as well. We
needed to decouple these and this code looks good (I like Hannes
suggestions too.)

One step closer to being able to package everything separately! :)

Thanks,
David Hurley
On Jan 12, 2014 7:19 AM, "Thomas Hunziker" notifications@github.com wrote:

I found two issues why the featured view didn't work.

• One in the plugin because it checks for a specific context and the featured was missing.
• The other is in the featured view itself, where the non-existant $this->params was passed to the onContentPrepare event. It should be $item->params instead. That's actually an unrelated bug [image: ]

I'll update the PR with those changes. Thanks for the good testing!

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

As mentioned on Tracker: Also Featured is now working.

Cool!

Leo Lammerink
On 1/12/2014 7:45 PM, David Hurley wrote:

This is great Thomas! I would like to test this afternoon as well. We
needed to decouple these and this code looks good (I like Hannes
suggestions too.)

One step closer to being able to package everything separately! :)

Thanks,
David Hurley
On Jan 12, 2014 7:19 AM, "Thomas Hunziker" notifications@github.com
wrote:

I found two issues why the featured view didn't work.

• One in the plugin because it checks for a specific context and the featured was missing.
• The other is in the featured view itself, where the non-existant $this->params was passed to the onContentPrepare event. It should be $item->params instead. That's actually an unrelated bug [image: ]

I'll update the PR with those changes. Thanks for the good testing!

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

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

Great addition Thomas !

One suggestion proposal to completely uncouple com_content from plg_content_contact :

com_content should actually become completely unaware of plg_content_contact, and that seems with your PR to be almost the case, which is great

However 3 small areas remain that could make it completely decoupled:

1) The html generated in case of a contact_link present in content should become part of the contact content plugin.

2) The data (link) stored for the contact link, while ok to be in the contents table, is still managed by com_content instead of the plugin

3) The settings for it are still generated by com_content instead of the plugin.

Those 3 things are not mandatory for this step in the right direction, but could become part of it, either in this PR or as separate feature.

The Observer pattern (Observer relation to be defined by the plg_content_contact plugin), applied to the Model for "2" and View for "1", as well as the extensions ability of JForms for "3" could be elegant solutions to this,

I actually like Beat's suggestions and care for point 1 and 3 especially. Think that settings indeed should be completely of com_content and be in the plugin. I am not a coder but assume that should not be too hard probably?

In favour of backwards compatibility and not introducing a completely new system in a maintenance release, I would not move the contact specific HTML code out now, but do that in a bigger change later. If we introduce a change like this, we should have an overall strategy for likewise implementations and I don't see us having that right now.

• 1 Did not think that far (backwards compatibility) . Makes sense to introduce that maybe in 4.0 or whatever we are going to call it? On 1/12/2014 8:11 PM, Hannes Papenberg wrote: > > In favour of backwards compatibility and not introducing a completely > new system in a maintenance release, I would not move the contact > specific HTML code out now, but do that in a bigger change later. If > we introduce a change like this, we should have an overall strategy > for likewise implementations and I don't see us having that right now. > > — > Reply to this email directly or view it on GitHub > #2256 (comment). >

As said, my suggestions could be different PRs and done later.

Not sure why changing differently the already changed lines in this PR would affect BC.

Making it completely uncoupled would allow typically to write other plugins that link to the author's twitter, or add a Facebook icon for the author profile, or link it to your favorite social component installed instead of (or in addition) of contact link. Actually the author text can already be changed onPrepareContent trigger. But, if the Observable implementation of the View is done right, it would allow very easily adding features to Content too.

That is a fair statement Beat and I like the suggestions of the other
plugins. That indeed would enhance and improve very much the
opportunities. If that can be achieved simple now we should do it me
think to avoid 'forgetting it" afterwards when we have committed current
patch however I also see the advantage of committing it now since it
finally resolves a long standing issue we have had I am not sure what is
best but again I leave that to people who know code and how hard or
simple it would be to implement your good suggestions now Beat....
Priority should be that this is being committed now and we polish it in
a later stage.... Opinions?
On 1/12/2014 8:15 PM, beat wrote:

As said, my suggestions could be different PRs and done later.

Not sure why changing differently the already changed lines in this PR
would affect BC.

Making it completely uncoupled would allow typically to write other
plugins that link to the author's twitter, or add a Facebook icon for
the author profile, or link it to your favorite social component
installed instead of (or in addition) of contact link. Actually the
author text can already be changed onPrepareContent trigger. But, if
the Observable implementation of the View is done right, it would
allow very easily adding features to Content too.

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

It's true that it could be improved. The idea was mainly to decouple the thing in a backward compatible way. It got a big PR already by doing that.
The ability to create links for components like CB instead of com_contact is a nice sideeffect which can be improved for sure. I like the ideas Beat mentioned but don't think they should go into this PR.

The data (link) stored for the contact link, while ok to be in the contents table, is still managed by com_content instead of the plugin.

@beat I'm not sure I understand you correct here. Com_content only stores the user. There is no info about a contact here. The plugin looks that up in the contact table and adds the id and link to the article $item.

The parameters need to stay in com_content for now simply for BC reasons. Also how would you store them in the plugin? Currently you can enable/disable the linking for com_content globally, on a per menuitem basis and per article. You couldn't do that in a plugin.

@Hackwar Good ideas. Will probably do in the evening

And as soon as you are done I will test ;-)
On 1/12/2014 8:48 PM, Thomas Hunziker wrote:

@Hackwar https://github.com/Hackwar Good ideas. Will probably do in
the evening

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

Caching the query (thanks @Fedik) and other improvements suggested by @Hackwar.
Ready for testage

@test
Works like a charm

@Bakual
Please update versions, date and copyright for the new files.

Can you explain what is '{"mode":"1"}' for in the sql? I do not see any params in the xml

Also, I suggest to modify the JGLOBAL_LINK_AUTHOR_DESC Tip for "Link Author" by adding with < strong > the fact that the plugin has to be enabled.

@test
this works fine. have not seen any B/C issue in multilingual.
Shall we reserve this for 3.3?

@test

+1 for the suggestion to reserve that for 3.3. Let's get this in the oven first?

Shall we reserve this for 3.3?

I've asked in PLT but didn't get a response so far. Imho it could go into 3.2 as it should be full BC, but since it's also technically a new feature it may be better in 3.3.

Please update versions, date and copyright for the new files.

Updated

Can you explain what is '{"mode":"1"}' for in the sql? I do not see any params in the xml

Copy-paste error. Removed

Also, I suggest to modify the JGLOBAL_LINK_AUTHOR_DESC Tip for "Link Author" by adding with < strong > the fact that the plugin has to be enabled.

Changed. But needs a review by someone who is native english. @brianteeman can you maybe have a look?

Thanks @brianteeman. Updated with your suggestions.

We don't need another Test for this do we?

Leo
On 1/13/2014 7:07 PM, Thomas Hunziker wrote:

Thanks @brianteeman https://github.com/brianteeman. Updated with
your suggestions.

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

We don't need another Test for this do we?

For language string changes it's usually enough if someone with enough english knowledge looks over it before merging.

I looked over it but maybe "Godfather" (smile) Brian should have the
final words here? ;-)
On 1/13/2014 8:14 PM, Thomas Hunziker wrote:

We don't need another Test for this do we?

For language string changes it's usually enough if someone with enough
english knowledge looks over it before merging.

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

Another suggestion: add a post-installation message. People NOT using the linked user may be real happy to know that disabling the plugin will make their site faster. :)

we could do that in a new PR after this one is merged. I'd rather have it separate from this one.

People NOT using the linked user may be real happy to know that disabling the plugin will make their site faster.

Actually, it will not make their sites really faster. If the "Link Author" parameter is disabled, the plugin will not execute the query. Thus the only gain would be that the plugin doesn't get loaded at all if it's disabled, which is a really small gain and probably not measurable.
I'm not sure if that really justifies a post-installation message.

No need for post-install message according to me
On 1/14/2014 11:04 PM, Thomas Hunziker wrote:

People NOT using the linked user may be real happy to know that
disabling the plugin will make their site faster.

Actually, it will not make their sites really faster. If the "Link
Author" parameter is disabled, the plugin will not execute the query.
Thus the only gain would be that the plugin doesn't get loaded at all
if it's disabled, which is a really small gain and probably not
measurable.
I'm not sure if that really justifies a post-installation message.

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

Thanks. No need indeed. Merged.