API Throw deprecation warnings for bad configuration #10702

GuySartorelli · 2023-02-23T23:23:56Z

Parent issue

Throw deprecation warning for common misconfiguration in CMS4 #10692

src/Control/HTTPApplication.php

michalkleiner · 2023-02-23T23:38:17Z

With the number of items we want to alert people to and the potential to have more of them in the future, do we want to create a new final class where these would be homed and just call it from wherever needed?
Or any other place other than inside the HTTPApplication class, it just doesn't feel right here.

GuySartorelli · 2023-02-23T23:26:15Z

thirdparty/swiftmailer/Swift/Transport/MailTransport.php

+        Deprecation::withNoReplacement(function () {
+            Deprecation::notice('4.12.0', 'Will be replaced with symfony/mailer', Deprecation::SCOPE_CLASS);
+        });


I figure this is the appropriate message rather than "Will be removed without equivalent functionality" because symfony/mailer comes with a suported sendmail DSN config out of the box: sendmail://default

Just realised this is for MailTransport - yeah no replacement here, there is no mail() support in symfony/mailer

Message should be along the lines of "There is no mail() in CMS 5, use sendmail or SMTP instead"

Should probably put the same deprecation notices in the MailInvoker + SimpleMailInvoker __construct() as well

Ooohhh, I thought this was for sendmail, didn't realise it was for mail().
Updated messages.

GuySartorelli · 2023-02-23T23:31:57Z

src/Control/HTTPApplication.php

+
+            if (!$hasDeprecatedStrategy && method_exists($protectedResolutionStrat, 'getResolutionFileIDHelpers')) {
+                foreach ($protectedResolutionStrat->getResolutionFileIDHelpers() as $helper) {
+                    if ($helper instanceof NaturalFileIDHelper) {


Don't just check for !($helper instanceof HashFileIDHelper) here because that will raise deprecation warnings for people doing their own crazy magic and having their own custom file resolution strategies. We're pretty much just trying to catch people who started a new project before 4.5 when installer first slapped in the new strategy config override.

src/Control/HTTPApplication.php

GuySartorelli · 2023-02-23T23:45:46Z

With the number of items we want to alert people to and the potential to have more of them in the future, do we want to create a new final class where these would be homed and just call it from wherever needed?

I expect this method to just be thrown away when we merge up to 5.0, since at that point this behaviour should all be removed and there's no sense having the method around doing a no-op. And I can't think of any reason we'd need to call it from anywhere else - the whole point of putting it here is that we know it's only going to be called the one time for each request. We don't want duplicate deprecation warnings.

michalkleiner · 2023-02-23T23:54:22Z

I expect this method to just be thrown away when we merge up to 5.0, since at that point this behaviour should all be removed and there's no sense having the method around doing a no-op.

And when CMS6 comes by, we will be reintroducing something similar and end up with something different again. I know CMS6 is relatively far, but at the same time it's not, it will fly by faster than we think.
I'm just thinking since you find a good place where to call it from and how to handle these things, the mechanism is worth preserving I'd say.

And I can't think of any reason we'd need to call it from anywhere else - the whole point of putting it here is that we know it's only going to be called the one time for each request. We don't want duplicate deprecation warnings.

The point where it's called from is fine.

GuySartorelli · 2023-02-23T23:57:00Z

And when CMS6 comes by, we will be reintroducing something similar and end up with something different again

I honestly don't see that as a problem. Two years from now we might not even remember this mechanism is here, and we risk having dead code and doing the same thing in a different way anyway. That said, if you still think this is worth keeping, I'll add a "Do not remove this method even if it's doing a no-op" to the phpdoc for it and update our docs about how to add deprecation notices in hopes we look there come CMS 6.

Edit: Actually with the change to docs it seems likely we will reuse this and the more I think about it the more you're right. I'll do that.

michalkleiner · 2023-02-24T00:03:37Z

Appreciate the reflection on this 👍. Could possibly add a note to the Deprecation class itself that this warning mechanism exists so we have one more place to remind us of it.

GuySartorelli · 2023-02-24T00:08:14Z

Could possibly add a note to the Deprecation class itself that this warning mechanism exists so we have one more place to remind us of it.

I've opted to instead link to the docs about deprecation notices in general, which is where I'll mention the mechanism. Link to the docs seems more useful overall I think.

I've also updated that class's phpdoc quite a bit since it was very out of date.

src/Control/HTTPApplication.php

emteknetnz · 2023-02-27T04:34:49Z

src/Control/HTTPApplication.php

+
+        // This change of defaults has no other deprecation notice being emitted currently.
+        $project = new Module(BASE_PATH, BASE_PATH);
+        if (RESOURCES_DIR !== '_resources' && $project->getResourcesDir() !== RESOURCES_DIR) {


This is confusing me. The PUBLIC_DIR if statement above uses || which is intuitive. However the if statement for RESOURCES_DIR uses && which is not intuitive.

Also the logic in Module::getResourcesDir() is

return isset($this->composerData['extra']['resources-dir']) ? $this->composerData['extra']['resources-dir'] : '';

So, are we deprecating NOT specifying what the resources-dir is in composer.json? Just having a quick look at a composer.json for a cms 5 install I have locally and I don't have it specified here and everything works normally.

Seems like we should be doing the opposite and simply not care what's in composer.json, cos it'll always be _resources going forward?

The PUBLIC_DIR if statement above uses || which is intuitive. However the if statement for RESOURCES_DIR uses && which is not intuitive.

These conditions are doing two different things.

if (PUBLIC_DIR !== 'public' || Director::publicDir() !== PUBLIC_DIR) {

If PUBLIC_DIR isn't public, we have a problem. If Director::publicDir() is giving a value other than PUBLIC_DIR, we have a problem.

if (RESOURCES_DIR !== '_resources' && $project->getResourcesDir() !== RESOURCES_DIR) {

RESOURCES_DIR is allowed to be "_resources", and $project->getResourcesDir() is allowed to not match RESOURCES_DIR (e.g. if not set) - but if RESOURCES_DIR isn't "_resources" then $project->getResourcesDir() must match that value. If it doesn't, it means they're probably using the old default value and haven't explicitly declared they want to keep it.

So, are we deprecating NOT specifying what the resources-dir is in composer.json?

Only if they're using something other than the default, but yes.

RESOURCES_DIR gets set in constants.php, where it'll read composer.json resources-dir or default to resources (CMS 4) or _resources (CMS 5)

Maybe I'm reading this wrong, though maybe all we need here is

if ($project->getResourcesDir() === '')

Meaning you haven't specified it in composer.json and are still using the old 'resources' default? But is that actually a problem? If you don't specify it in CMS 5 and it'll nicely use the _resources default.

So, people will be upgrading from 4.13 to 5.0.0, they turn on deprecation notices, and we then asking people to specifying resources-dir ... only to then not require it in CMS 5?

What happens when you upgrade from CMS 4 -> CMS 5 and you haven't specified resources-dir in composer.json? Does the old resources dir get nuked and a new _resources dir get populated?

I have some feeling that not specifying _resources in composer.json causes them to break right now anyway

I'm not sure we need this deprecation warning here? Maybe we need to split this off to another card here to explore this further?

It was a reaction to Steve's "the message ... needs to tell users they need to set the value..." where it seemed to me that suggesting resources could be there to reduce upgrade pain.

Offer its configurability via composer.json, suggest _resources, and discourage resources (if needs to be mentioned at all).

I think the simplified if ($project->getResourcesDir() === '') has much better intent

I disagree, but it's really not worth holding up the PR for this so I've made the change.

The message in the Deprecation::notice() needs to be changed tell users that they need to set this value in composer.json, preferably to _resources, but realistically resources to reduce upgrade pain?

I don't think it does. It includes a link to the changelog, which is the source of truth for whatever advice we want to give about how to handle this scenario. If you think the information in the changelog is insufficient please note what specific changes should be made in the changelog (note it in the issue since you've already merged the docs PR) and I'll update the changelog.

Oh yeah I remember the "pages named resources" thing from years ago. Yeah, really bad to have it called 'resources' because it's a common name for a page, would be ideal if anyone still on that migrated off that.

we shouldn't exercise the option of coming back to resources

Yeah that would be great, should just force it to be _resources and that's it, but probably too late now that we're in the beta.

The changelog is a bit confusing in this context because they're on a 4.13 project, but the changelog is for 5.0.0. In 4.13 we ideally want them to add "_resources" to their composer.json, though in the 5.0.0 changelog we say "If your composer.json file has its extra.resources-dir key set to _resources, you can remove that now." ... so that's confusing

I'd like to just make the message be "add _resources to your composer.json now" - but I've got no idea what happens when you do that - we probably should split this off as a new card to investigate the amount of friction there is around the resources folder

we probably should split this off as a new card to investigate the amount of friction there is around the resources folder

Whether the readme needs to be updated or further investigation is required as a separate card, that should not hold up this PR.

Yeah I'll raise a card to investigate further, keep as is for now

#10710

emteknetnz · 2023-02-28T06:14:57Z

thirdparty/swiftmailer/Swift/Transport/MailTransport.php

+        Deprecation::withNoReplacement(function () {
+            Deprecation::notice('4.12.0', 'Will be replaced with symfony/mailer', Deprecation::SCOPE_CLASS);
+        });


Just realised this is for MailTransport - yeah no replacement here, there is no mail() support in symfony/mailer

Message should be along the lines of "There is no mail() in CMS 5, use sendmail or SMTP instead"

GuySartorelli marked this pull request as ready for review February 23, 2023 23:24

GuySartorelli mentioned this pull request Feb 23, 2023

Throw deprecation warning for common misconfiguration in CMS4 #10692

Closed

michalkleiner reviewed Feb 23, 2023

View reviewed changes

src/Control/HTTPApplication.php Show resolved Hide resolved

GuySartorelli commented Feb 23, 2023

View reviewed changes

GuySartorelli force-pushed the pulls/4/deprecated-misconfigurations branch from 0817fcc to 6ec8820 Compare February 23, 2023 23:47

GuySartorelli force-pushed the pulls/4/deprecated-misconfigurations branch 2 times, most recently from b464ff0 to e01670f Compare February 24, 2023 00:03

GuySartorelli force-pushed the pulls/4/deprecated-misconfigurations branch from e01670f to 0c07750 Compare February 24, 2023 00:07

This was referenced Feb 24, 2023

DOC Document deprecation notices for behaviour silverstripe/developer-docs#170

Merged

DOC Document some behaviours that aren't supported anymore silverstripe/developer-docs#171

Merged

GuySartorelli force-pushed the pulls/4/deprecated-misconfigurations branch from 0c07750 to 48d2b62 Compare February 24, 2023 00:50

GuySartorelli commented Feb 24, 2023

View reviewed changes

src/Control/HTTPApplication.php Outdated Show resolved Hide resolved

GuySartorelli marked this pull request as draft February 24, 2023 01:09

GuySartorelli force-pushed the pulls/4/deprecated-misconfigurations branch from 48d2b62 to 7903c88 Compare February 24, 2023 02:43

GuySartorelli mentioned this pull request Feb 24, 2023

API Explicitly deprecate legacy file resolution silverstripe/silverstripe-assets#543

Merged

GuySartorelli marked this pull request as ready for review February 24, 2023 02:47

emteknetnz requested changes Feb 27, 2023

View reviewed changes

GuySartorelli force-pushed the pulls/4/deprecated-misconfigurations branch from 7903c88 to 60f1457 Compare February 28, 2023 03:26

emteknetnz requested changes Feb 28, 2023

View reviewed changes

API Throw deprecation warnings for bad configuration

441af97

GuySartorelli force-pushed the pulls/4/deprecated-misconfigurations branch from 60f1457 to 441af97 Compare February 28, 2023 21:00

emteknetnz mentioned this pull request Feb 28, 2023

Investigate what happens when "resources" changes to "_resources" #10710

Open

emteknetnz approved these changes Feb 28, 2023

View reviewed changes

emteknetnz merged commit 5295ba6 into silverstripe:4 Feb 28, 2023

emteknetnz deleted the pulls/4/deprecated-misconfigurations branch February 28, 2023 22:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Throw deprecation warnings for bad configuration #10702

API Throw deprecation warnings for bad configuration #10702

GuySartorelli commented Feb 23, 2023

michalkleiner commented Feb 23, 2023 •

edited

Loading

GuySartorelli Feb 23, 2023

emteknetnz Feb 28, 2023

emteknetnz Feb 28, 2023

GuySartorelli Feb 28, 2023 •

edited

Loading

GuySartorelli Feb 23, 2023

GuySartorelli commented Feb 23, 2023

michalkleiner commented Feb 23, 2023

GuySartorelli commented Feb 23, 2023 •

edited

Loading

michalkleiner commented Feb 24, 2023

GuySartorelli commented Feb 24, 2023

emteknetnz Feb 27, 2023

emteknetnz Feb 27, 2023 •

edited

Loading

GuySartorelli Feb 27, 2023

GuySartorelli Feb 27, 2023 •

edited

Loading

emteknetnz Feb 27, 2023

michalkleiner Feb 28, 2023

GuySartorelli Feb 28, 2023

emteknetnz Feb 28, 2023

GuySartorelli Feb 28, 2023 •

edited

Loading

emteknetnz Feb 28, 2023 •

edited

Loading

emteknetnz Feb 28, 2023

API Throw deprecation warnings for bad configuration #10702

API Throw deprecation warnings for bad configuration #10702

Conversation

GuySartorelli commented Feb 23, 2023

Parent issue

michalkleiner commented Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli commented Feb 23, 2023

michalkleiner commented Feb 23, 2023

GuySartorelli commented Feb 23, 2023 • edited Loading

michalkleiner commented Feb 24, 2023

GuySartorelli commented Feb 24, 2023

Choose a reason for hiding this comment

emteknetnz Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Feb 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

emteknetnz Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalkleiner commented Feb 23, 2023 •

edited

Loading

GuySartorelli Feb 28, 2023 •

edited

Loading

GuySartorelli commented Feb 23, 2023 •

edited

Loading

emteknetnz Feb 27, 2023 •

edited

Loading

GuySartorelli Feb 27, 2023 •

edited

Loading

GuySartorelli Feb 28, 2023 •

edited

Loading

emteknetnz Feb 28, 2023 •

edited

Loading