-
Notifications
You must be signed in to change notification settings - Fork 822
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
Various deprecations and a few features #11365
Various deprecations and a few features #11365
Conversation
src/Core/CoreKernel.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't think of a way to unit test this, unfortunately, since the kernel gets booted as part of unit tests before we get a chance to set this
protected function getRegisteredController($baseUrlPart) | ||
{ | ||
Deprecation::notice('5.4.0', 'Will be removed without equivalent functionality to replace it'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to wrap in withNotReplacement()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? It's not called from anywhere?
public function __construct() | ||
{ | ||
parent::__construct(); | ||
Deprecation::notice('5.4.0', 'Will be replaced with symfony/console commands', Deprecation::SCOPE_CLASS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to wrap in withNoReplacement as there's no replacement in CMS 5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? We don't use this anywhere?
/** | ||
* Get the amount of time inbetween two datetimes. | ||
*/ | ||
public static function getTimeBetween(DBDateTime $from, DBDateTime $to): string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this being added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see the PR description
* | ||
* @deprecated 5.4.0 Will be replaced with SilverStripe\Dev\Command\DbBuild::lastBuilt() | ||
*/ | ||
public static function lastBuilt() | ||
{ | ||
Deprecation::withNoReplacement(function () { | ||
Deprecation::notice( | ||
'5.4.0', | ||
'Will be replaced with SilverStripe\Dev\Command\DbBuild::lastBuilt()' | ||
); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* | |
* @deprecated 5.4.0 Will be replaced with SilverStripe\Dev\Command\DbBuild::lastBuilt() | |
*/ | |
public static function lastBuilt() | |
{ | |
Deprecation::withNoReplacement(function () { | |
Deprecation::notice( | |
'5.4.0', | |
'Will be replaced with SilverStripe\Dev\Command\DbBuild::lastBuilt()' | |
); | |
}); | |
*/ | |
public static function lastBuilt() | |
{ |
Class is already deprecated and method name will be the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a static method. It's possible to call this method without instantiating the class, which means we need a deprecation warning in the method itself.
@@ -55,9 +59,22 @@ class DatabaseAdmin extends Controller | |||
|
|||
/** | |||
* Config setting to enabled/disable the display of record counts on the dev/build output | |||
* @deprecated 5.4.0 Will be replaced with SilverStripe\Dev\Command\DbBuild.show_record_counts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @deprecated 5.4.0 Will be replaced with SilverStripe\Dev\Command\DbBuild.show_record_counts |
Class is already deprecated and property is the same name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussion can be in #11365 (comment) given config is effectively the same as a static method.
@@ -38,6 +41,7 @@ class DatabaseAdmin extends Controller | |||
|
|||
/** | |||
* Obsolete classname values that should be remapped in dev/build | |||
* @deprecated 5.4.0 Will be replaced with SilverStripe\Dev\Command\DbBuild.classname_value_remapping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @deprecated 5.4.0 Will be replaced with SilverStripe\Dev\Command\DbBuild.classname_value_remapping |
Class is already deprecated and property is the same name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussion can be in #11365 (comment)
9ebfe7c
to
dbe567d
Compare
dbe567d
to
b621cae
Compare
Rebased to resolve conflict |
The commits in this PR are intentionally separated, do not squash.
DatabaselessKernel
and merge that functionality intoCoreKernel
instead - and addBaseKernel::getBooted()
(see src/Cli/Sake.php)ArrayLib
(see src/Cli/LegacyParamArgvInput.php)DBDateTime
(see src/Dev/BuildTask.php)Makes sense to add the non-API-breaking new features into this minor instead of having it in the 6 PR.
Issues