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

Replace deprecated sort rand with shuffle #720

Merged
merged 2 commits into from
Aug 18, 2023
Merged

Replace deprecated sort rand with shuffle #720

merged 2 commits into from
Aug 18, 2023

Conversation

3Dgoo
Copy link
Contributor

@3Dgoo 3Dgoo commented Aug 10, 2023

No description provided.

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.

Hi, thanks for this.
It would have been good to include more information about the rand function being deprecated such as a link to a relevant changelog - but regardless, this seems like a sensible change.

Please retarget the pr to the 3.12 branch, as it seems like this is a change that should be available for CMS 4 projects as well.

@3Dgoo 3Dgoo changed the base branch from 4 to 3.12 August 11, 2023 10:41
@3Dgoo 3Dgoo changed the base branch from 3.12 to 4 August 11, 2023 10:42
@kinglozzer
Copy link
Member

@GuySartorelli shuffle() was added in 4.5.0 (I only looked it up because I had no idea the method existed!) - if we were to merge this into the 3.12 branch, would we want to bump the framework requirement? Or should we just keep it simple and merge into 4?

@GuySartorelli
Copy link
Member

GuySartorelli commented Aug 18, 2023

Ohhhh. That makes it a bit of a pain. I guess we could do feature detection and check if the method exists but given there was no info provided about the deprecated functionality this is avoiding, let's keep it simple and leave it targeting 4. We can always have a new pr for 3.12 if it becomes a problem
for someone.
Thanks for spotting that @kinglozzer

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.

This is fine as is for the branch tis targeting.

@GuySartorelli GuySartorelli merged commit b9c535a into silverstripe:4 Aug 18, 2023
12 checks passed
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.

3 participants