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

Add a hardDeprecationErrors setting (default false) that throws an exception if a deprecation error is logged #3972

Closed
wants to merge 2 commits into from

Conversation

khalwat
Copy link
Contributor

@khalwat khalwat commented Mar 10, 2019

Puts Craft in "hard mode" where any deprecation error that gets logged will throw an exception if the general config setting hardDeprecationErrors is true (defaults to false).

It can be convenient to throw exceptions whenever a deprecation is encountered, so that you're forced to go in and fix them.

@@ -781,6 +782,19 @@ public function expressionFunction($expression, $params = [], $config = []): Exp
return new Expression($expression, $params, $config);
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

This seems unrelated to the rest of the pull request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah sorry, I must have screwed something up when I synced my fork with the remote

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a PR that @brandonkelly ended up renaming to create() -- I'd love to know what I'm doing wrong when syncing my fork with the upstream, sorry for the hassles.

Copy link
Member

Choose a reason for hiding this comment

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

@khalwat Best bet is to make branches for each PR, so you're never committing directly to your fork’s master or develop branch - those only get pulled from upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes total sense, thank you @brandonkelly

@chasegiunta
Copy link
Contributor

Was just looking for this! Might I suggest a different name, though: errorOnDeprecations to follow the naming scheme that Craft is already running with (actions, eg. useProjectConfigFile, allowAdminChanges, useEmailAsUsername, etc.)

…ception if a deprecation error is logged

Signed-off-by: Andrew Welch <[email protected]>
Signed-off-by: Andrew Welch <[email protected]>
@brandonkelly
Copy link
Member

We decided to add this as a configurable property on the Deprecator service, rather than a config setting.

To have Craft throw exceptions for deprecation warnings, add this to config/app.php after updating to the the next release (3.1.18):

return [
    'components' => [
        'deprecator' => [
            'throwExceptions' => true,
        ],
    ],
];

@khalwat
Copy link
Contributor Author

khalwat commented Mar 13, 2019

yep, that's a better way to do it imo

@timkelty
Copy link
Contributor

timkelty commented Apr 12, 2019

FWIW, I'm guessing most folks would want to implement this way, to not inadvertently break things in production:

return [
    'components' => [
        'deprecator' => [
            'throwExceptions' => YII_DEBUG,
        ],
    ],
];

@khalwat
Copy link
Contributor Author

khalwat commented Apr 12, 2019

^ good point @timkelty

I'm guessing this just happened :)

@timkelty
Copy link
Contributor

Nope! just scurrrrrrred 👻

@brandonkelly
Copy link
Member

config/app.php can be a multi-environment config as well, so you could also do this:

return [
    '*' => [
        // global config...
    ],
    'dev' => [
        'components' => [
            'deprecator' => [
                'throwExceptions' => true,
            ],
        ],
    ],
];

@timkelty
Copy link
Contributor

@brandonkelly TBH, I'm never quite sure of the merging recursion method when you do that…

e.g –

return [
    '*' => [
        'components' => [
            'deprecator' => [
                'logTarget' => 'logs',
            ],
        ],
    ],
    'dev' => [
        'components' => [
            'deprecator' => [
                'throwExceptions' => true,
            ],
        ],
    ],
];

Would components.deprecator.logTarget be logs in dev, or does it get overwritten? This might be documented somewhere but I can't find it 👺

@khalwat
Copy link
Contributor Author

khalwat commented Apr 16, 2019

It works as you'd expect (no different than without the multi-env keys) but in general I'm moving towards env vars and flat configs. At least for now. :)

@bossanova808
Copy link

I just want to chime in and thank @khalwat and @brandonkelly & all in here for this...this is just great.

I didn't see it documented anywhere (probably just missed it, it's hard for a non full time dev to keep up with everything!) - but Andrew accidentally showed it to me while helping with something else (does that guy ever sleep?!).

Turning these hard errors on I was able to finally trace two buried-very-deeply deprecation errors that I'd been looking for over several hours, for literally months now - in just a few minutes.

So - thanks all, and especially Andrew. You people are awesome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants