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

FIX Wrap deprecated config with no replacement #10704

Merged

Conversation

GuySartorelli
Copy link
Member

We shouldn't see deprecation warnings for this config call unless we explicitly set $showNoReplacementNotices to true

Parent issue

@@ -704,7 +704,7 @@ public static function baseFolder()
*/
public static function publicDir()
{
$alternate = self::config()->uninherited('alternate_public_dir');
$alternate = Deprecation::withNoReplacement(fn() => self::config()->uninherited('alternate_public_dir'));
Copy link
Member

Choose a reason for hiding this comment

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

Please use the standard non-short function() here that we used for everything else

I know this works, however if we need to Ctrl-F to find everything later for whatever reason this make it harder to find

Copy link
Member Author

@GuySartorelli GuySartorelli Feb 27, 2023

Choose a reason for hiding this comment

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

"standard" here is subjective. Short arrow function syntax was only introduced in PHP 7.4, so we couldn't use this until 4.11.0 which dropped support for PHP 7.3 and that was just in June last year. I'd say the only reason using a full function () { } syntax for these is "standard" is because we haven't been able to use the new syntax until very recently.
I would prefer to keep this new syntax here - in my mind "we haven't used this syntax before" is a really bad reason to not use new syntax, given the only reason we hadn't used it before it we literally couldn't do that without breaking things.

if we need to Ctrl-F to find everything later for whatever reason this make it harder to find

What exactly would you have in your search that makes this harder to find? You'd have to be looking for something really specific to the point of being unreliable anyway.

Copy link
Member Author

@GuySartorelli GuySartorelli Feb 27, 2023

Choose a reason for hiding this comment

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

If you're going to insist on this change, we probably need some more opinions from the core committers about whether this will be a rule going forward - and if it is, we should add a linting rule to catch it.

Copy link
Contributor

@michalkleiner michalkleiner Feb 27, 2023

Choose a reason for hiding this comment

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

In this case, I'd keep it "the old way" and for CMS5 we can do it the new way, but at the same time there is already a mix of both, so not a big deal. Wouldn't hold the PR for this.

Re: the search argument you can find either, especially when these closures aren't that common.

Then as a bonus, we can create a few MNT cards for using things like str_contains instead of strpos !== false, null safe object operator ?-> etc. Lead the way with modern code.

Copy link
Member

Choose a reason for hiding this comment

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

All I'm saying is make it match the syntax of the other several dozen other withNoReplacement replacement calls so things are consistent.

Deprecation::withNoReplacement(function () { won't match it which is what I'd personally be liable to copy paste

Copy link
Member Author

@GuySartorelli GuySartorelli Feb 27, 2023

Choose a reason for hiding this comment

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

Since there's two of you saying to use the old way for this, and one of you isn't gonna merge until I do, I'll make that change.
I do think we should start using new syntax though - be forewarned that for things like this I'm going to start pushing more and more for the new syntax to be used.

Having cards to update some of the syntax as well is a good idea - but lets look at that sort of thing once we've got 5 stable released and have some more time for that sort of enhancement.

And for what it's worth, that search will fail anyway for anything where we've had to split it across multiple lines. By the time you need regex to handle that you may as well have Deprecation::withNoReplacement\((\s*function|fn)
But the function/fn part is irrelevant because any call to Deprecation::withNoReplacement() is going to have a callback passed into it, and none of our usage has any callable that isn't an anonymous function. So just search for Deprecation::withNoReplacement(

Copy link
Member Author

Choose a reason for hiding this comment

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

Anyway I've made the change now

@GuySartorelli GuySartorelli force-pushed the pulls/4/wrap-deprecation branch from 08bb5c3 to 0fc9f94 Compare February 27, 2023 03:53
@emteknetnz
Copy link
Member

@GuySartorelli GuySartorelli force-pushed the pulls/4/wrap-deprecation branch from 0fc9f94 to d525375 Compare February 27, 2023 04:36
@GuySartorelli
Copy link
Member Author

Fixed

@emteknetnz emteknetnz merged commit 6669d54 into silverstripe:4 Feb 27, 2023
@emteknetnz emteknetnz deleted the pulls/4/wrap-deprecation branch February 27, 2023 05:13
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.

3 participants