support storing attributes as \SAML2\XML\saml\Attribute objects

[dub357]

This is a fairly big change, but should be backwards compatible. In conjunction with a change to simplesamlphp (for which another pull request will be generated), it will allow for finer control of attributes (specifically NameFormats and FriendlyNames) since simplesamlphp doesn't currently support attribute friendly names or different name formats per attribute

[thijskh]

Hi, thanks for the PR. I will need to review it into more detail. It would be very helpful/a good start if you could update your branch to current master so it's rid of conflicts and the travis build passes.

[thijskh]

Do you think you can rebase it on master? That would be helpful.

[dub357]

Yes I will rebase to latest current when I get some time

[coveralls]

Coverage decreased (-0.6%) to 67.847% when pulling 52fbdf3 on dub357:master into 6b7c762 on simplesamlphp:master.

[tvdijen]

Something must have gone wrong with the merge from master, because I see two completely different versions of the constructor in src/SAML2/XML/saml/AttributeValue.php..
That's what's failing the 7.2 check now

[dub357]

I don't see multiple versions of a constructor in AttributeValue...the travis build is complaining about "Calling assert() with a string argument is deprecated". This was related to writing the original code based on a older version of the library. I'm working through the travis issues now. I'm not sure how half this stuff ever passed as I'm being forced to fix things that I didn't write LOL

[dub357]

Oh I think I see what you meant...the constructor assert was different in my master vs simplesamlphp master. That should be fixed now. Seems like this codeclimate stuff is new...I don't remember seeing that before. Are those issues something I should be concerned with? I'm not sure how its complaining about some of the stuff it thinks is an issue as there are much longer and complex methods in simplesaml master compared to the stuff I changed.
Just to give some background...I need to have finer control over attribute friendly names and name formats so I introduced the ability the control them. While in the process I figured that making the assertion use the \SAML2\SAML\saml\Attribute objects would be a good idea as well since I see the codebase headed that way... I can roll this back if needed

[tvdijen]

Yes, that was exactly what I meant, but I missed for a sec that those differences are part of the PR 😆
Codeclimate issues can safely be ignored.. It is indeed something new.
Perhaps a squash of all those satisfy-travis commits to keep things clean?

[dub357]

You may have to teach me how to do that lol. I'm fairly new to github, although I used distributed version control systems before (mainly Mercurial). I couldn't even figure out how to pull simplesamlphp/saml2 master changes back into my clone (dub357/saml2) using the github interface so I was forced to work out all the conflicts and fix things using the built-in editing capability. Because of this, it was kind of a trial and error process getting the travis build to pass because its difficult to tell where the offending whitespaces were (when sometimes a tab was all that was needed to be removed) and thats why there were so many commits to fix them. All the whitespace requirements are a bit over the top if you ask me LOL - whats wrong with a single space at the end of a line or no space between a close parentheses and an open bracket?
How do I go about "squashing" all those commits into one?

[thijskh]

You can do that by running git rebase -i on your branch.

I've disabled codeclimate. I'm very allergic to "failing" builds for weeks on end as that teaches everyone to ignore the ❌'s and that's very bad.

[tvdijen]

Unit tests are crucial for PR's, as well as coding standards and syntax-checking...
Code quality (complexity, comprehensibility) not so much... That's more something main developers have to keep an eye on if you ask me...
When looking at the codeclimate issues, that's not something we're gonna fix in this PR.

@dub357: If you're not familiar with the git command-line, I can help you out

[dub357]

@tvdijen I think I need some help. I tried to do this rebase stuff, but I dont think it worked as planned.
As I stated before, I worked out the conflicts and whitespace issues directly on my forked version of the repository (via the github UI), not on my local checkout of it. This means all those commits are in my central github repo. After I made them, I did a pull on my local repo to pull the commits in. So I was running into problems trying to rebase because git was saying that my local repo is up to date. I tried to combine the commits into one which worked, but when I pushed them to the github repo, it doesn't look like it worked...it just showed up as an additional commit. What am I doing wrong?

[tvdijen]

Hehe, somehow you've added an empty commit...
I've tried squashing everything for you, but unfortunately git is not my friend today (or any other day for that matter)...

[dub357]

So I dont want my inability to squash these dumb whitespace commits to hold up the review and/or pull of this. I'm kinda new to GitHub and I have a pull request submitted for simplesaml (simplesamlphp/simplesamlphp#691) that unfortunately now includes commits that depend on this code :(
I didn't realize that any commits I push to my fork while I have an open pull request will get added to that pull request. I apologize and you can close that one if necessary. Maybe I just need to figure out how to revert those commits in my simplesamlphp fork until this pull request is merged

[tvdijen]

This is weird, I don't know what happened here, but.. The PR closed while attempting to squash those commits and I can't reopen it anymore..
Anyway, I have a squashed version of your patch here: tvdijen@40851a6
You can cherry-pick it and reopen this PR, or I can make a new PR for you @dub357.

[dub357]

So I can’t figure out how to reopen this PR and it doesn’t look like I can create another PR without maybe adding another commit to my fork . Looks like another recent PR changing the Assertion class has been pulled in the meantime so there probably more conflicts to deal with.
At this point I’m a bit frustrated with Github and the pull request process so I guess do with it what you like. I was hoping to make some significant progress in making simplesaml more flexible and compatible with regards to attribute handling - something it significantly lacks - but maybe changing to use AttributeValue objects was too much for the codebase to digest at once. If you don’t pull in my code, can you (or someone else who can speed up the pull request process) get some code in that will handling getting and setting attribute name formats and friendly names in a similar manner?

[tvdijen]

I feel your pain @dub357, I had the same frustrations with this PR last night..
But there's a new PR now that is rebased against master, no conflicts and all tests pass, so let's stay positive :D

...and it doesn’t look like I can create another PR without maybe adding another commit to my fork

Within your fork, you should make separate feature branches for that.. Don't put everything in master.