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

Add IsFirst/IsLast methods to match SS5 conventions (closes #1274) #1275

Open
wants to merge 1 commit into
base: 5
Choose a base branch
from

Conversation

kinglozzer
Copy link
Member

@kinglozzer kinglozzer commented Nov 12, 2024

Description

Deprecates First() and Last() in favour of IsFirst() and IsLast(), as these methods are intended to override the built-in iteration methods of those names.

Manual testing steps

Check the value of $IsFirst and $IsLast in elemental block templates

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

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Makes sense, thanks for submitting this. I've filled in the missing information from the PR template for you this time around.

Please also add a PHPDoc comment describing what the isFirst() and isLast() methods do.
Please also make a PR to developer-docs adding notes about these new deprecations to the 5.4.0 changelog.

src/Models/BaseElement.php Show resolved Hide resolved
src/Models/BaseElement.php Outdated Show resolved Hide resolved
src/Models/BaseElement.php Outdated Show resolved Hide resolved
@GuySartorelli
Copy link
Member

Please also add either the API or NEW prefix to the commit message.

@GuySartorelli
Copy link
Member

There are tests for the original methods. Please add tests for the new methods as well.

@kinglozzer
Copy link
Member Author

Have made the requested amends, docs PR: silverstripe/developer-docs#631

Comment on lines +216 to +217
$this->assertFalse($element->Last());
$this->assertTrue($element2->Last());
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$this->assertFalse($element->Last());
$this->assertTrue($element2->Last());
$this->assertFalse($element->IsLast());
$this->assertTrue($element2->IsLast());


/**
* Returns true if this is the last element rendered in the ElementalArea
* @return bool
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @return bool

Sorry, should have caught this last time. We don't need the PHPDoc if it doesn't give any additional context over the actual type hint.

@@ -1232,21 +1232,41 @@ public function getPageTitle()
}

/**
* @return boolean
* Returns true if this is the first element rendered in the ElementalArea
* @return bool
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @return bool

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