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

fix: correct $segment for PopulateTask per docs #49

Merged
merged 1 commit into from
Feb 9, 2023
Merged

fix: correct $segment for PopulateTask per docs #49

merged 1 commit into from
Feb 9, 2023

Conversation

wilr
Copy link
Member

@wilr wilr commented Oct 10, 2022

@GuySartorelli as per silverstripe/silverstripe-staticpublishqueue@2f05ec1 it looks like this class was upgraded without one defined but the documentation refers to the old version. To review if we should mark this as a API change before merge. I don't think this would be as drastic as the above one since it's unlikely anyones running it in production (only perhaps for demo sites)

@GuySartorelli
Copy link
Member

IMO This is a breaking change and will need to target a new major branch once one is created.
For whatever reason, the master branch in this repository is actually the branch for the current major line, so breaking changes can't be introduced to it.

That said, like you've pointed out there's a much lower chance of this being run in production than the queued jobs module so I'll let someone else make the call.

@michalkleiner
Copy link
Contributor

The repo was originally DNA's so I think the master branch situation is a remnant of the transition to silverstripe org not having a major line branch created.
I'm happy to merge the PR since the change is not changing any functionality and the adoption of the module is probably reasonably small, more so its use in production.

@madmatt
Copy link
Member

madmatt commented Feb 9, 2023

Yep I agree with @michalkleiner, merging this now.

@madmatt madmatt merged commit 7797c69 into silverstripe:master Feb 9, 2023
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.

4 participants