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

API Add new deprecation notices. #10691

Merged
merged 1 commit into from
Feb 15, 2023

Conversation

GuySartorelli
Copy link
Member

These are removed in CMS 5.

Note there have been so many new deprecations which we already haven't added to the CMS 4.13 changelog so there's no point manually adding this there, we're going to have to do some batch card to add all the ones we've missed anyway and we'll likely just automate that.

Parent issue

Comment on lines -118 to +119
Deprecation::notice('5.0', __CLASS__ . ' is deprecated, use ' . Reader::class . ' instead');
Deprecation::notice('4.13.0', 'Use ' . Reader::class . ' instead', Deprecation::SCOPE_CLASS);
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm explicitly not wrapping this one in Deprecation::withNoReplacement() because there are only a few instances of it in use in our code, and we can wrap those instead. This seems likely to be used pretty frequently in projects so it's good for their uses to pop up in the main deprecation notices.

Comment on lines -257 to +261
$csv = new CSVParser(
$filepath,
$this->delimiter,
$this->enclosure
);
$delimiter = $this->delimiter;
$enclosure = $this->enclosure;
$csv = Deprecation::withNoReplacement(function () use ($filepath, $delimiter, $enclosure) {
return new CSVParser($filepath, $delimiter, $enclosure);
});
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the only usage of this class in our code that I could find.

Since this whole method is deprecated, it's easier to just wrap this instantiation than to replace it with the Reader class. It's gone in CMS 5 anyway.

@@ -24,7 +25,10 @@ protected function setUp(): void
public function testParsingWithHeaders()
{
/* By default, a CSV file will be interpreted as having headers */
$csv = new CSVParser($this->csvPath . 'PlayersWithHeader.csv');
$filepath = $this->csvPath;
Copy link
Member

@emteknetnz emteknetnz Feb 14, 2023

Choose a reason for hiding this comment

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

Instead of wrapping calls here, instead change setUp() to the following:

    protected function setUp(): void
    {
        parent::setUp();
        $this->csvPath = __DIR__ . '/CsvBulkLoaderTest/csv/';
        if (Deprecation::isEnabled()) {
            $this->markTestSkipped('Test calls deprecated code');
        }
    }

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

These are removed in CMS 5.
@emteknetnz emteknetnz merged commit ab566b0 into silverstripe:4 Feb 15, 2023
@emteknetnz emteknetnz deleted the pulls/4/deprecations branch February 15, 2023 00:26
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.

2 participants