fixed bug in JUser Bind not checking value correct #1743

Merged
merged 1 commit into from Dec 9, 2012

6 participants

@fanno

Also added userbind test ? not 100% sure this would work as this is my first ...time making this. i need someone that understand the test envioment to try it.

Hope you understand

-Thanks

@fanno fanno referenced this pull request Dec 8, 2012
Closed

JUser bind #1741

@elinw

The test is failing right now but that's because the bug isn't fixed. What you want to do is put your test into the same branch as the bug fix so that it passes when it is run on the patched code.

@fanno
@fanno

if that's not what you mean i don't know because both were for joomla:staging ?

@pasamio

The bug fix should be in the same request as the pull request as the test proving the bug fix works. Same branch, same pull request and it gets all merged at the same time.

@fanno

well i was asked to make it later, how do i update a pull request or should i just make a new one ? and what branch should it be for ?

@pasamio

GitHub will update an open pull request automatically with new commits made to a branch. Just merge this branch into your earlier branch that you used to make that pull request and then push it. GitHub will automatically detect the change and update the original pull request.

@fanno

like so ? sorry i am new to pull requests =(

@fanno

this also means i can close the other request right ?

@pasamio

Yes you can close the other one and edit this one to update the title

@fanno

here by done, thanks for baring with me

@pasamio

Can you double check the indentation is correct and that you're using tabs? It looks like the indentation is different between the existing lines of code and the added lines of code.

@fanno

ahh now i know what you mean by indentation, i use space in my phpstorm setup i think..

http://tinypic.com/r/4kelaq/6

@fanno

is there a phpstorm config file for joomla that i can use to keep same coding syntax ? and what not ?

@elkuku
Joomla! member

Sure, the auto formatter files are here: https://github.com/joomla/coding-standards/tree/master/IDE
You might also be interested in: http://docs.joomla.org/Joomla_CodeSniffer
And, well: http://docs.joomla.org/?search=git

@fanno

thanks =)

@pasamio

Looks like Jenkins is reporting an issue with the unit test:
http://developer.joomla.org/pulls/pulls/1743.html

It's probably choking on the colon character which I don't recall as being a word character (I think it's alphanumeric and underscores).

We're also racking up the commits, it'd be great once we get the unit tests all sorted that it was all squashed back into one commit:
http://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits

@fanno

hmm i thought it was only testing with the info that i provided? inside ?

maybe

    $this->assertTrue(
        (strlen($testUser->username) >= 1 && strlen($testUser->username) <= 150)
    );

    $this->assertTrue(
        (strlen($testUser->password) >= 1 && strlen($testUser->password) <= 100)
    );

or something ?

@fanno

how do other characters get in to the test ? should it not only be testing with the info i hard coded in the function ?

@elinw

Am I right that we have been counting on JForm to do the checking for us and not dealing with the fact that it could be done in code?

Other tests are probably somewhere calling bind.

Try using one of the users in the xml file i.e. create the JUser with a specific known id.

@fanno

i don't know i founded this "bug" when i was adding custom code to check if a username was longer than 5 characters and founded that it seemed to be a bug, i do not know if jform should do this check already.

if that so then i guess this code is redundant all to gather ?

@elinw

No, I think it's good to do. The save is happening here and you would want to make sure that it is right, and you see that obviously that was the intent at some point. I think you can just redo the test a bit, don't worry about the matching just focus on the length issue. Anytiime JUser is involved things are apt to be messy.

@fanno

There removed the regex and just testing length

@elinw

So what you want is to put in a string that is more than 150 characters and have it return the substring that is the first 150 characters. That's how I would set up the test.

    $longString = '12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890';
    $expectedPassword = '1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890';
    $expectedUsername = '123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890';

And then see if what you actually get matches what you expected.

ha you beat me to it!

@pasamio

The reality is the password length doesn't matter, it'll get hashed and stored regardless. Once upon a time I'm sure it mattered in the situation where the password wasn't hashed and perhaps there was an overflow bug in PHP with the MD5 functions at some point (though unlikely since it's used for larger files as well) but there isn't a way the end user can make the password longer than the length it is now because it gets hashed inside bind.

@pasamio

Can you squash these commits into one? You'll need to force push back up to your repo (git push -f ...) after the rebase:
http://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits

@elinw

That's great, running and passing. :) My only suggestion is that you actually check the truncation to make sure you get the correct value and also that you add an example that doesn't need truncation. Then it will be a nice complete test. Personally I think in usability terms saving a password that is different than what the user entered is a problem but mainly we catch that stuff in jform before the fact anyway.

I remember a big debate about the length of both fields in 1.5 because i think there are some very long names, utf8 characters and probably some other issues.

@fanno

yes tomorrow

@fanno

i cant figure out the squashing =(

@jfusion jfusion added userbind test ? not 100% sure this would work as this is my fir…
…st time making this. i need somone that understand the test envioment to try it.

Fixed issue with the password and username check is not preformed for new users because the values are not yet set, Fixed by moving the check below setProperties

fixed in indentation guess it had to be tabs ?

fixed in indentation guess it had to be tabs ?

changed from regex to just check the length
6532b56
@fanno

there we go i think ? did that give the effect you were looking for ?

@elinw

Beautiful :)

@fanno

Thanks for baring with me, my first time doing anything this "advanced"

@pasamio pasamio merged commit 4d43daf into joomla:staging Dec 9, 2012
@fanno

hmm i guess i should have removed the @todo Implement testBind(). ?

@pasamio

Well I think you've end to end done everything short of making a new branch so if you can remember it all you're set. In future what I suggest you do is start with a new branch.

I wrote this up, let me know if it's useful:
http://docs.joomla.org/Git_branching_quickstart

@pasamio

Yeah, maybe removing the TODO is a good idea :) another pull request? 👍

@fanno

i added a new request that includes this and test for two more functions
#1745

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment