API Deprecate SecurityAdmin API

[emteknetnz]

Issue https://github.com/silverstripeltd/product-issues/issues/627

[GuySartorelli]

I don't think that's how withNoReplacement() is meant to work.... isn't it intended simply to wrap calls to deprecated code? So you'll wrap new GroupImportForm() in a callback passed into withNoReplacement() but not the deprecation notice itself.

We have been a little inconsistent with it (I did find a couple of other usages like this one) - but most "without equivalent functionality" deprecation notices are not directly wrapped like this.
We should probably unwrap the ones that are wrapped.

[GuySartorelli]

Applies to others in this PR and probably in other related PRs.

[emteknetnz]

When I originally wrote this it was intended to wrap calls, though then we realised later works the same directly wrapping notices. When deprecating classes we exclusively use the wrap notices method. Both ways of doing it are legitimate.

All that wrapping calls does is surpress any notices, so, it's the same thing. Aesthetically it's not as great, though there's zero practical difference for end-users.

I'm not going to wrap calls just for the sake of aesthetics.

[GuySartorelli]

Is this going to suppress notices if a community module or project uses the import form directly for some reason? If so, we shouldn't wrap the notice call.

[emteknetnz]

Yes. But there's no replacement. That's the point of suppressing notice where there's "no replacement", because there's no migration to go to while you're still on CMS 4, so you'll just get spammed with messages that you cannot do anything about.

You can show suppressed messages via Deprecation::enable(true); rather than the regular Deprecation::enable()

I've updated the PR to developer-docs to note this - https://github.com/silverstripe/developer-docs/pull/150/files#diff-0d0c6e36c4cca126ccacdc42ab8b4765656ee67badcedc061f2e0c21129bfddc

[GuySartorelli]

I would argue that if someone is using something deprecated, the warning should absolutely always be given, and then they can note it down so they know what they need to do when they upgrade to CMS 5. People are only going to enable deprecation notices when they're doing a major upgrade, so it makes sense to tell them everything they're using that's deprecated. They can then make a note of it, wrap it in withNoReplacement() and continue.

In either case we need to make sure we're being consistent. Either always wrap it, or never wrap it. I'm in favour of never wrap it but if you want to update this PR to always wrap notices for API that has no replacement and open one for all other modules which have them I'll accept that too.

[emteknetnz]

It's simply not worth updating it to be consistent at this stage. This is code that's CMS 4 only and won't have much in the way of active development on it. It makes zero practical difference.

The vast amount Deprecation::withNoReplacement() has already been merged and was done in issues such as silverstripe/silverstripe-framework#10542. I'm not updating that, not at this late stage.

[GuySartorelli]

Most cases, as I mentioned, do not use withNoReplacement(). Consistency in this case is to not wrap it in this PR.

[emteknetnz]

Deprecation::SCOPE_CLASS are always in __construct(), so we never wrap calls - this is consistent with the rest of the code base

All the other things on SecurityAdmin are controller endpoints, so we'd need to wrap whatever calls controller endpoints in framework, which is just wrong.

I think the way this PR has it is correct

[GuySartorelli]

I'll accept this - we can just make it clear in the documentation how to deal with these.