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

Remove deprecated AttributeRealm authproc #1186

Merged
merged 1 commit into from Feb 14, 2020
Merged

Remove deprecated AttributeRealm authproc #1186

merged 1 commit into from Feb 14, 2020

Conversation

@tvdijen
Copy link
Member

@tvdijen tvdijen commented Aug 12, 2019

It was deprecated since 1.15 and there is a proper alternative in place, so I'd like to remove this in 1.18

@tvdijen tvdijen requested a review from jaimeperez Aug 12, 2019
@tvdijen tvdijen added this to the 1.18 milestone Aug 12, 2019
@tvdijen
Copy link
Member Author

@tvdijen tvdijen commented Aug 26, 2019

@jaimeperez you said you wanted to review this.. Could you?

Loading

@thijskh
Copy link
Member

@thijskh thijskh commented Aug 28, 2019

It's a really small thing... is it urgent to drop it or can we do that at 2.0 instead?

Loading

@tvdijen
Copy link
Member Author

@tvdijen tvdijen commented Aug 28, 2019

We can have it any way we want, however, my concern is that we're putting off way too much for 2.0..
The migration for our users is going to be quite a thing, with Twig, translations, routing, event dispatching, a possible PHP upgrade if they are on 5.x, and what not more..
Should we perhaps consider an intermediate major with just Twig, translations and PHP version bump and leave the rest for 3.0? I think it would help us progress

Loading

@thijskh
Copy link
Member

@thijskh thijskh commented Aug 28, 2019

The best way to progress in my opinion is to release 1.18 now and move any further work for 1.19 or 2.0

Loading

@jaimeperez
Copy link
Member

@jaimeperez jaimeperez commented Aug 29, 2019

I agree with Thijs. I'm even considering having 1.17.6 released with fixes for the few bugs we have identified lately.

The entire point of 2.0 is to make all those changes that are not backwards-compatible there. Removing this authproc in a minor version (1.18) could break existing deployments, and I would therefore prefer to avoid that. Do we have a log message in place when using this authproc saying it has been deprecated and it will be removed? If so, does it point out to the alternative?

Also, the reason why we haven't released 1.18 yet is that I would like to have everything in place that people will need to effectively migrate to 2.0. A painful migration is precisely what I'm trying so hard to avoid here, and that's why we're making our own lives miserable by supporting both new and old functionalities at the same time.

The new routing mechanism is a key part for 2.0, IMO. We need to have that in place, together with a fully working new user interface, in a 1.x release. My goal was that 1.18 would be that release, but if you think it's not realistic and we should have it released already and postpone the routing refactor to 1.19, I'm fine with that too.

Loading

docs/simplesamlphp-authproc.md Show resolved Hide resolved
Loading
@tvdijen tvdijen removed this from the 1.18 milestone Aug 29, 2019
@tvdijen tvdijen added this to the 2.0 milestone Aug 29, 2019
@tvdijen tvdijen requested a review from jaimeperez Aug 31, 2019
Copy link
Member

@jaimeperez jaimeperez left a comment

This gets my full blessing for 2.0! Thanks Tim!

Loading

@tvdijen tvdijen force-pushed the remove_attributerealm branch from ed99b54 to 4090424 Nov 22, 2019
@tvdijen tvdijen added this to In progress in 2.0 release via automation Nov 22, 2019
@tvdijen tvdijen force-pushed the remove_attributerealm branch from 4090424 to 4c63885 Dec 24, 2019
@codecov
Copy link

@codecov codecov bot commented Dec 24, 2019

Codecov Report

Merging #1186 into master will decrease coverage by 0.46%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1186      +/-   ##
============================================
- Coverage     36.85%   36.39%   -0.47%     
+ Complexity     3777     3762      -15     
============================================
  Files           138      136       -2     
  Lines         11451    11401      -50     
============================================
- Hits           4220     4149      -71     
- Misses         7231     7252      +21
Impacted Files Coverage Δ Complexity Δ
lib/SimpleSAML/Utils/EMail.php 0% <0%> (-59.1%) 30% <0%> (-2%)
modules/core/lib/Controller/Login.php 74.24% <0%> (-0.76%) 18% <0%> (ø)
lib/SimpleSAML/Store/SQL.php 73.29% <0%> (-0.63%) 42% <0%> (ø)
lib/SimpleSAML/Utils/XML.php 63.52% <0%> (ø) 74% <0%> (ø) ⬇️
lib/SimpleSAML/Metadata/Signer.php 0% <0%> (ø) 32% <0%> (ø) ⬇️
modules/core/lib/Controller/Exception.php 0% <0%> (ø) 9% <0%> (-1%) ⬇️
lib/SimpleSAML/Kernel.php
lib/SimpleSAML/Command/RouterDebugCommand.php
lib/SimpleSAML/Console/Application.php
lib/SimpleSAML/HTTP/Router.php 0% <0%> (ø) 9% <0%> (?)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1e3604...5f856fa. Read the comment docs.

Loading

@tvdijen tvdijen force-pushed the remove_attributerealm branch from 4c63885 to 5f856fa Feb 14, 2020
@tvdijen tvdijen merged commit e7ff7c6 into master Feb 14, 2020
1 of 5 checks passed
Loading
2.0 release automation moved this from In progress to Done Feb 14, 2020
@tvdijen tvdijen deleted the remove_attributerealm branch Feb 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2.0 release
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants