Skip to content

Propagate Symfony responses throughout the code#1760

Merged
tvdijen merged 56 commits into
masterfrom
symfony_responses
May 12, 2023
Merged

Propagate Symfony responses throughout the code#1760
tvdijen merged 56 commits into
masterfrom
symfony_responses

Conversation

@tvdijen

@tvdijen tvdijen commented Feb 13, 2023

Copy link
Copy Markdown
Member

This PR is far from complete, but it's big enough to review as it is already. Consider this a first batch.
I've verified that basic SP/IDP redirections still work.

@tvdijen tvdijen requested a review from thijskh February 13, 2023 23:24
@codecov

codecov Bot commented Feb 13, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1760 (06346cc) into master (21118cd) will increase coverage by 0.69%.
The diff coverage is 31.14%.

❗ Current head 06346cc differs from pull request most recent head 0168fcb. Consider uploading reports for the commit 0168fcb to get more accurate results

Additional details and impacted files 
@@             Coverage Diff              @@
##             master    #1760      +/-   ##
============================================
+ Coverage     44.67%   45.37%   +0.69%     
- Complexity     3697     3710      +13     
============================================
  Files           162      162              
  Lines         12359    12407      +48     
============================================
+ Hits           5522     5630     +108     
+ Misses         6837     6777      -60

@tvdijen tvdijen force-pushed the symfony_responses branch 6 times, most recently from 164c168 to 94dc48b Compare February 16, 2023 23:58
thijskh
thijskh reviewed Feb 17, 2023
View reviewed changes

@thijskh thijskh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely a good cleanup. Does touch a lot of things so may require intensive testing though.

Comment thread modules/exampleauth/src/Auth/Source/External.php Outdated
Comment thread modules/exampleauth/src/Controller/ExampleAuth.php Outdated
Comment thread src/SimpleSAML/Utils/HTTP.php Outdated
@tvdijen

tvdijen commented Feb 17, 2023

Copy link
Copy Markdown
Member Author

This is about as much as I can do right now.. If we want to take the next step, we also have to update the saml2-lib bindings to return a Symfony-response in their ::send() methods.

tvdijen
tvdijen commented Feb 19, 2023
View reviewed changes
Comment thread src/SimpleSAML/Auth/Source.php
* @param array &$state Information about the current authentication.
*/
public static function completeAuth(array &$state): void
public static function completeAuth(array &$state): Response

@tvdijen tvdijen Feb 19, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side-quest: should we change the signature to non-static & protected, since all calls to this are made by child-classes?

@tvdijen tvdijen force-pushed the symfony_responses branch 13 times, most recently from dd713cc to fb54ee5 Compare February 26, 2023 11:36
@tvdijen tvdijen force-pushed the symfony_responses branch 2 times, most recently from 5bb3659 to 7e6527e Compare March 4, 2023 10:41
@tvdijen tvdijen force-pushed the symfony_responses branch from 9830410 to 0d079ba Compare May 3, 2023 20:22
@tvdijen tvdijen force-pushed the symfony_responses branch 3 times, most recently from e53f5d2 to 47cffb7 Compare May 9, 2023 15:25
@tvdijen tvdijen force-pushed the symfony_responses branch from 3ddc430 to 51e0dc3 Compare May 9, 2023 15:48
@tvdijen

tvdijen commented May 11, 2023

Copy link
Copy Markdown
Member Author

I thoroughly tested this PR in conjunction with a new version of the saml2-lib.
The lib was changed to:

  • Bump PHP functional level to 8.0 and take advantage of it's features
  • Alter it's namespace to SimpleSAML\SAML2
  • Alter the binding-classes to work with PSR-17 HTTP messages as required by this PR

@thijskh OK to merge?

@sinteur

sinteur commented May 11, 2023

Copy link
Copy Markdown

I'd recommend a 'yes' to thijskh :-D

@jaimeperez

jaimeperez commented May 12, 2023

Copy link
Copy Markdown
Member

I haven't been able to look closely into everything, but this kind of development is indeed a massive improvement! 👏

@tvdijen tvdijen force-pushed the master branch 5 times, most recently from 3f79544 to 7204202 Compare May 12, 2023 13:59
@tvdijen tvdijen merged commit a6458f5 into master May 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@thijskh thijskh thijskh left review comments

Assignees

No one assigned

Labels

enhancement maintainability

Projects

None yet

Milestone

3.0

Development

Successfully merging this pull request may close these issues.

4 participants

@tvdijen @sinteur @jaimeperez @thijskh