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.

Sign up for GitHub

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

Jump to bottom

ENH More standardisation #223

Merged
merged 2 commits into from
Feb 15, 2024
Merged

ENH More standardisation #223

merged 2 commits into from
Feb 15, 2024

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented Feb 15, 2024 •
edited
Loading

Description

Two commits:

  1. The bulk of the standardisation - removing some more unused code, adding a test, etc.
  2. Just moving some methods around (and some very minor changes to phpdoc). I did this separately so it'll be easier to review the actual changes being made.

Issues

Pull request checklist

  • The target branch is correct
  • All commits are relevant to the purpose of the PR (e.g. no debug statements, unrelated refactoring, or arbitrary linting)
    • Small amounts of additional linting are usually okay, but if it makes it hard to concentrate on the relevant changes, ask for the unrelated changes to be reverted, and submitted as a separate PR.
  • The commit messages follow our commit message guidelines
  • The PR follows our contribution guidelines
  • Code changes follow our coding conventions
  • This change is covered with tests (or tests aren't necessary for this change)
  • Any relevant User Help/Developer documentation is updated; for impactful changes, information is added to the changelog for the intended release
  • CI is green

@GuySartorelli GuySartorelli mentioned this pull request Feb 15, 2024
Closed
13 tasks
GuySartorelli
GuySartorelli commented Feb 15, 2024
View reviewed changes
src/Models/FileLink.php Outdated
public function getDefaultTitle(): string
protected function getDefaultTitle(): string
Copy link
Member Author

Choose a reason for hiding this comment

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

Never called outside of Link models except in tests.
Everywhere else should use getTitle() to get the real title.

GuySartorelli
GuySartorelli commented Feb 15, 2024
View reviewed changes
src/Models/FileLink.php Outdated
if (!$file->exists()) {
if (!$file?->exists()) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Matches how it's used elsewhere in this class.

GuySartorelli
GuySartorelli commented Feb 15, 2024
View reviewed changes
src/Models/Link.php Outdated
Comment on lines 5 to 8
use InvalidArgumentException;
use ReflectionException;
use SilverStripe\Core\ClassInfo;
use SilverStripe\Core\Injector\Injector;
Copy link
Member Author

Choose a reason for hiding this comment

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

Not used anymore

GuySartorelli
GuySartorelli commented Feb 15, 2024
View reviewed changes
src/Models/Link.php
Comment on lines -77 to -80
public function scaffoldLinkFields(array $data): FieldList
{
return $this->getCMSFields();
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Unused code

GuySartorelli
GuySartorelli commented Feb 15, 2024
View reviewed changes
src/Models/Link.php Outdated
Comment on lines 82 to 83
public function LinkTypeHandlerName(): string
/**
* Get the react component used to render the modal form.
*/
public function getLinkTypeHandlerName(): string
Copy link
Member Author

Choose a reason for hiding this comment

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

Getters should be prefixed with get

GuySartorelli
GuySartorelli commented Feb 15, 2024
View reviewed changes
src/Models/Link.php
@@ -132,91 +133,9 @@ public function onBeforeWrite(): void
parent::onBeforeWrite();
}

function setData($data): Link
Copy link
Member Author

Choose a reason for hiding this comment

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

Unused method

GuySartorelli
GuySartorelli commented Feb 15, 2024
View reviewed changes
src/Models/Link.php
@@ -237,19 +156,6 @@ public function jsonSerialize(): mixed
return $data;
}

public function loadLinkData(array $data): Link
Copy link
Member Author

Choose a reason for hiding this comment

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

Unused method

GuySartorelli
GuySartorelli commented Feb 15, 2024
View reviewed changes
tests/php/Models/FileLinkTest.php
Comment on lines -21 to +22
$link = FileLink::create();
$link = new FileLink();
Copy link
Member Author

Choose a reason for hiding this comment

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

Tests should avoid using injector when they're not explicitly testing injector

GuySartorelli
GuySartorelli commented Feb 15, 2024
View reviewed changes
tests/php/Models/FileLinkTest.php
@@ -35,4 +36,26 @@ public function testGetDescription(): void
$link->write();
$this->assertSame('file-b.png', $link->getDescription());
}

public function testGetDefaultTitle(): void
Copy link
Member Author

Choose a reason for hiding this comment

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

Matches a test in SiteTreeLinkTest

emteknetnz
emteknetnz approved these changes Feb 15, 2024
View reviewed changes
Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Code change look fine, have smoke tested locally

@emteknetnz emteknetnz merged commit c75615f into silverstripe:4 Feb 15, 2024
12 checks passed
@emteknetnz emteknetnz deleted the pulls/4/more-standardisation branch February 15, 2024 04:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emteknetnz emteknetnz emteknetnz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@GuySartorelli @emteknetnz