Note about the Travis CI error: we think it is happening because PHPUnit removed their link to PEAR recently. We think it will automatically sorts itself out when the patch is updated and the build check runs again. Let's hope and see :-)

@fanno Could you please update your PR against the latest staging. I currently get this error:

error: patch failed: plugins/authentication/joomla/joomla.php:38
error: plugins/authentication/joomla/joomla.php: patch does not apply

This means that the file is out-of-sync with staging. Thank you.

I am noob so i am nut sure how to do this?

I am noob so i am nut sure how to do this?

hmm if you work with git e.g. on the commandline the tool is called rebase http://git-scm.com/docs/git-rebase

If you don't know what i mean i guess the easiyest way to fix it is to redo the PR against the current staging branche to resolve the merge conflicts

i guess i could try and re do it AGAIN, but if its not going to get merged like all the other "count less" things i have been toled/asked to make. then it is just a waste of my time..

In any case i think that

$app->triggerEvent('onUserAfterAuthenticate', array($credentials, $options, &$response));

For two factor would be a better option so that all auth plugins can make use it this. and not having to make their own.

-Thanks

Do you have another username? I dont see any other pull requests from you (open or closed)

its on the joomla framework mainly

@fanno I will check with the PLT to see what they think about this PR. If we decide to move forward, let's redo the PR then.

@nikosdion Hey Nic, what do you think of this as the author of 2FA :) Thanks for your input.

What exactly is this PR supposed to do? We have a vague title about a "fix", no description of the problem issue being "fixed", just a link to a huge thread that goes on forever. The actual post being referenced talks about using a separate event for 2FA which is called for all authentication plugins but this is not addressed in the code I'm reading. Previous posts talk about the return value of the plugin and go on forever and ever about a trivial, inconsequential matter, this PR does seem to address it in a very wrong way that has incidentally very little to do with 2FA. Anyway let me comment on both possible intentions of the PR. You're not making my life easy :p

Let's forget that this PR doesn't try to create a new plugin event for 2FA. I disagree with the concept anyway. It should be up to authentication plugins to decide if they need to support 2FA or not. For example, one could write an authentication plugin which uses a secure card reader to authenticate the user. This use case would not require 2FA. But since this login presumably requires special hardware and software on the computer to work they might also want to enable 2FA on the same site e.g. with YubiKey, for legacy username/password authentication. Or maybe you want a transparent authentication for your RESTful API, in which case you might want to sidestep 2FA (think about how GitHub uses tokens for 2FA-less authentication against the RESTful API).

On top of that I see another thing on that thread, the return value of user authentication plugins. But this PR doesn't fix that. You only had to change four (4) lines. Instead you converted the PlgAuthenticateJoomla's code into a mess of nested if-blocks that avoid using return; like the plague. Why are you doing that? A return with no value and not using return have the same effect as return null in PHP. You have worsened the code readability. It was bad as it was, now it's impossible to read unless printed in a really long piece of paper and used as a carpet in the longest hallway you have in your office (yeah, I'm exaggerating, but it's still hard to read even on my 27" external monitor, let alone the 13" built-in screen) :)

If anything, we should get rid of the gazillion nested if-blocks and, most importantly, their else statements. If I were to go into the trouble I'd have also created a base class for Authentication plugins and move the TFA code into a protected method inside it. Of course that does have a minor b/c impact (at some point auth plugins extending from JPlugin would cease functioning) but that's another story,

Just a side note. Next time you submit a PR please help us understand what you are doing. IMHO, a good PR should provide the following information:

• Executive summary. What the PR does in 30-50 words.
• Technical information. Analysis of why it previously didn't work and what you did to fix it.
• Testing instructions. Obviously, committing a PR without testing is not gonna happen.
• Backwards compatibility. Does it affect b/c? Document exactly how and your assessment of the impact of your PR to b/c.
• Language impact. Does it add, modify or remove language strings? I know, it sucks having to provide so much text with your code but such is life when you have to work with other humans who can't read your mind.

Correct, as you can see in the very first post i made the "only thing" the pr is for so that "phpstorm" and i assume other IDE don't complain.

So in essence i guess you could say it is a useless PR, it fixes nothing.

I only tryed to contribute something i founed, that's all..

Thanks! I now understand what you were trying to do. You should have edited this file and simply replace return false; and return true; with return;. You didn't have to rewrite the plugin.

I guess so, it just my own stupid mind? that that only like to have one return statement.

if return is changed to return; then @return boolean should be @return void or other way around the return statement never used for anything (if i remember correct.)

Yes, the docblock should declare @return void, this is the expected result.

FYI, this plugin has been around since Joomla! 1.0. Back in the day –at the top of my head I believe throughout Joomla! 1.5 as well– it was expected to return a boolean value: true for successful authentication, false otherwise. The plugin was refactored but since its return value was ignored nobody thought much about changing the docblock and return statements. When I made the 2FA PR I kept the same return values since I only looked at the docblock, not where this plugin is called from.

ok, so i change return statements to return; and change @return boolean to @return void or (assuming i had sycess by rebase tried) should i update this PR or whats the plan then?

I think it's easier to close this PR and open a new one. Remember to add the text "Replaces gh-3123" in your new PR so that all participants of the old, closed PR will get a notification about your new PR. If you're wondering, gh- followed by an issue or PR number (as displayed at the top of the GitHub page) creates a magic notification to everyone participating in the discussion of the referenced issue or PR. It's one of those small GitHub wonders :)

ok will do that

PSA: This PR has been replaced by its author with gh-6891 Discussion to be continued there.

@nikosdion Thank your stepping in here. Much appreciated.

Cheers :)