API Remove deprecated API #11524

GuySartorelli · 2025-01-06T03:20:43Z

Issue

Remove deprecated code .github#311

GuySartorelli · 2025-01-06T21:42:59Z

_config/encryptors.yml

-    md5:
-      'SilverStripe\Security\PasswordEncryptor_LegacyPHPHash': md5


Not keeping this key and setting it to PasswordEncryptor_PHPHash because md5 is inherently insecure, so anyone already using it should be prompted to consider changing to something else.

This, isn't just removing deprecated code? Seems like this is something else where we're making a decision that md5 is not good? Seems a bit out of scope?. Wouldn't this means there are other things to consider, for instance is it worth considering if sha1 and blowfish also up to scratch? Also there's an md5_v2.4 key below that's retained

This is literally just removing code. I have decided not to add other code because md5 sucks - but as far as this PR which is to remove deprecated code, that's exactly what is being done here.

I get that md5 isn't good, though we still have md5 support via the md5_v2.4 key below. Also, again, removing md5 support is out of scope since the scope of this issue is to only to remove deprecated code and we haven't "deprecated" md5 support.

Seems like the correct thing would be to retain the md5 key and just change its class to SilverStripe\Security\PasswordEncryptor_PHPHash like you did for sha1?

I wholeheartedly and completely disagree with every fibre of my being but I'll do it anyway so we can move on.

The implication to me is that the encryptor being deprecated means the config that instantiates it is also deprecated. Anyone who has explicitly set their project to use md5 will currently be getting a deprecation notice telling them that the legacy PHP hash encrpytor is deprecated.

I only migrated the sha1 key because that's still a sensible hash algorithm to use. md5 very very very much isn't. We should really remove it even from the encryptor we are keeping but that is farther out of scope than I want to do in this PR.

Anyway, md5 is back :(

The class which is deprecated is separate from the algorithm. People would also be getting deprecation notices for the sha1 algorithm as well though we're not removing that

I've backlogged a new issue for this #11530

GuySartorelli · 2025-01-06T22:06:44Z

src/Control/Director.php

-     *
-     * @return string|null null if not overridden, otherwise the actual value
-     */
-    public static function get_session_environment_type(?HTTPRequest $request = null)


This isn't used anywhere in core or supported modules. It was added in ca56e8d#diff-983f05f9a107b9bf66a1ed4230e524be82118dd71f4a795529f8794b762c8b47R1091 as a replacement for CoreKernel::sessionEnvironment(). It was removed in #10456.

It and this method seem to have only existed to support the isDev and isTest GET variables, which were intentionally removed in CMS 5.

isDev and isTest query string arguments have been removed due to security concerns (see ss-2018-005).

GuySartorelli · 2025-01-06T22:13:28Z

src/Control/Middleware/URLSpecialsMiddleware/SessionEnvTypeSwitcher.php

Should have been removed in #10456 back when we did CMS 5 work.
No need to mention this in the changelog, since this is dead code as the tests mentioned in the linked PR prove.

GuySartorelli · 2025-01-06T22:14:50Z

src/Control/HTTP.php

-    public static function urlRewriter($content, $code)
+    public static function urlRewriter($content, callable $code)
    {
-        if (!is_callable($code)) {
-            throw new InvalidArgumentException(
-                'HTTP::urlRewriter expects a callable as the second parameter'
-            );
-        }


Not really a deprecation but seems sensible to have an explicit typed parameter instead of an exception here.

GuySartorelli · 2025-01-06T22:15:49Z

src/Control/Middleware/URLSpecialsMiddleware.php

See comment about SessionEnvTypeSwitcher

GuySartorelli · 2025-01-06T22:17:50Z

src/Forms/Validation/CompositeValidator.php

@@ -4,7 +4,6 @@

 use InvalidArgumentException;
 use SilverStripe\Core\Validation\ValidationResult;
-use SilverStripe\Dev\Deprecation;


GuySartorelli · 2025-01-06T22:18:22Z

src/ORM/DataObject.php

-            $result = PolymorphicHasManyList::create($componentClass, $details['joinField'], static::class);
-            if ($details['needsRelation']) {
-                Deprecation::withSuppressedNotice(fn () => $result->setForeignRelation($componentName));
-            }
+            $relation = $details['needsRelation'] ? $componentName : null;
+            $result = PolymorphicHasManyList::create($componentClass, $details['joinField'], static::class, $relation);


setForeignRelation() is removed, and a new constructor arg is added in its place.

GuySartorelli · 2025-01-06T22:19:37Z

src/View/ThemeResourceLoader.php

@@ -9,7 +9,6 @@
 use SilverStripe\Core\Manifest\ModuleLoader;
 use SilverStripe\Core\Manifest\ModuleResourceLoader;
 use SilverStripe\Core\Path;
-use SilverStripe\Dev\Deprecation;


GuySartorelli · 2025-01-06T22:20:20Z

tests/php/Cli/SakeTest.php

@@ -3,7 +3,6 @@
 namespace SilverStripe\Cli\Tests;

 use PHPUnit\Framework\Attributes\DataProvider;
-use PHPUnit\Framework\Error\Deprecated;


GuySartorelli · 2025-01-06T22:20:54Z

tests/php/Forms/GridField/GridFieldTest.php

-        $obj = new GridField('testfield', 'testfield');
+        $list = ArrayList::create();
+        $obj = new GridField('testfield', 'testfield', $list);


Needed to avoid LogicException now that setThrowExceptionOnBadDataType() is removed from various components.

emteknetnz · 2025-01-07T02:38:57Z

_config/encryptors.yml

-    md5:
-      'SilverStripe\Security\PasswordEncryptor_LegacyPHPHash': md5


This, isn't just removing deprecated code? Seems like this is something else where we're making a decision that md5 is not good? Seems a bit out of scope?. Wouldn't this means there are other things to consider, for instance is it worth considering if sha1 and blowfish also up to scratch? Also there's an md5_v2.4 key below that's retained

emteknetnz · 2025-01-07T02:49:25Z

src/ORM/PolymorphicHasManyList.php

     */
-    public function __construct($dataClass, $foreignField, $foreignClass)
+    public function __construct(string $dataClass, string $foreignField, string $foreignClass, ?string $foreignRelation = null)


Why is a new param being added here?

Because that was always the intention. As per the deprecation notice:

@deprecated 5.2.0 Will be replaced with a parameter in the constructor

This was referenced Jan 6, 2025

FIX Stop calling deprecated API silverstripe/silverstripe-lumberjack#182

Merged

Remove deprecated code silverstripe/.github#311

Closed

API Remove deprecated API silverstripe/silverstripe-userforms#1361

Merged

GuySartorelli force-pushed the pulls/6.0/remove-deprecated branch 5 times, most recently from 6fb1e86 to 5169aa3 Compare January 6, 2025 22:10

GuySartorelli commented Jan 6, 2025

View reviewed changes

GuySartorelli mentioned this pull request Jan 6, 2025

API Deprecate API which will be removed in CMS 6 #11526

Merged

GuySartorelli force-pushed the pulls/6.0/remove-deprecated branch 2 times, most recently from 3e7be21 to e70fcc0 Compare January 7, 2025 01:06

emteknetnz requested changes Jan 7, 2025

View reviewed changes

API Remove deprecated API

003e119

GuySartorelli force-pushed the pulls/6.0/remove-deprecated branch from e70fcc0 to 003e119 Compare January 7, 2025 03:28

emteknetnz mentioned this pull request Jan 7, 2025

Remove md5 password encryption support #11530

Open

6 tasks

emteknetnz approved these changes Jan 7, 2025

View reviewed changes

emteknetnz merged commit e2a4804 into silverstripe:6.0 Jan 7, 2025
12 checks passed

emteknetnz deleted the pulls/6.0/remove-deprecated branch January 7, 2025 04:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Remove deprecated API #11524

API Remove deprecated API #11524

GuySartorelli commented Jan 6, 2025

GuySartorelli Jan 6, 2025

emteknetnz Jan 7, 2025

GuySartorelli Jan 7, 2025 •

edited

Loading

emteknetnz Jan 7, 2025 •

edited

Loading

GuySartorelli Jan 7, 2025 •

edited

Loading

emteknetnz Jan 7, 2025 •

edited

Loading

GuySartorelli Jan 6, 2025

GuySartorelli Jan 6, 2025

GuySartorelli Jan 6, 2025

GuySartorelli Jan 6, 2025

GuySartorelli Jan 6, 2025

GuySartorelli Jan 6, 2025

GuySartorelli Jan 6, 2025

GuySartorelli Jan 6, 2025

GuySartorelli Jan 6, 2025

emteknetnz Jan 7, 2025

emteknetnz Jan 7, 2025

GuySartorelli Jan 7, 2025

		md5:
		'SilverStripe\Security\PasswordEncryptor_LegacyPHPHash': md5

API Remove deprecated API #11524

API Remove deprecated API #11524

Conversation

GuySartorelli commented Jan 6, 2025

Issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

emteknetnz Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

GuySartorelli Jan 7, 2025 • edited Loading

Choose a reason for hiding this comment

emteknetnz Jan 7, 2025 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Jan 7, 2025 •

edited

Loading

emteknetnz Jan 7, 2025 •

edited

Loading

GuySartorelli Jan 7, 2025 •

edited

Loading

emteknetnz Jan 7, 2025 •

edited

Loading