Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make processor aware of assertion types #242

Merged
merged 1 commit into from
Aug 20, 2020

Conversation

MKodde
Copy link
Contributor

@MKodde MKodde commented Aug 19, 2020

Integrates #240 into master

@codecov
Copy link

codecov bot commented Aug 19, 2020

Codecov Report

Merging #242 into master will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #242      +/-   ##
============================================
+ Coverage     87.84%   87.97%   +0.12%     
- Complexity     2404     2406       +2     
============================================
  Files           166      166              
  Lines          6016     6020       +4     
============================================
+ Hits           5285     5296      +11     
+ Misses          731      724       -7     

@MKodde MKodde force-pushed the feature/integrate-processor-change branch 3 times, most recently from 74af94c to 6381665 Compare August 19, 2020 07:02
@MKodde MKodde requested a review from tvdijen August 19, 2020 07:05
@tvdijen
Copy link
Member

tvdijen commented Aug 19, 2020

It's a nitpick, but could you add a bunch of use-statements to the test-file and squash the full namespaced class names?

@MKodde
Copy link
Contributor Author

MKodde commented Aug 20, 2020

Absolutely. I thought the team preferred having less import statements in favor of instant readability what namespace a class hails from?

Both the Encrypted and regular Assertion classes should be passable in
the decryptAssertion method. If not, you would never be able to process
a SAML Response consisting of regular Assertion objects.

The test merely verifies the behaviour that was changed in this commit.
No additional processor tests where added. But that should be simple
enough in the future.

For some odd reason using a data provider for the
processor_correctly_encrypts_assertions caused issues in other tests.
The test suite does not seem to be fully idempotent.
@MKodde MKodde force-pushed the feature/integrate-processor-change branch from 6381665 to bd1fe04 Compare August 20, 2020 06:45
@MKodde MKodde merged commit ab6ec3e into master Aug 20, 2020
@MKodde MKodde deleted the feature/integrate-processor-change branch August 20, 2020 08:23
@tvdijen
Copy link
Member

tvdijen commented Aug 20, 2020

I've noticed that some of the analyzer tools do a better job with the use-statements in place..
We add the full namespaced classes for parameters to the phpdoc and i'm considering adding @uses tags for the others for instant readability..
It's hard to keep psr12 compliant if you put then inline in the code

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants