-
Notifications
You must be signed in to change notification settings - Fork 169
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
Bugfix: Updating BlogPostFilter::augmentSQL to make use of modern Versioned methods. #727
Bugfix: Updating BlogPostFilter::augmentSQL to make use of modern Versioned methods. #727
Conversation
Good morning! May I please get a status check on this pull request? cc: @GuySartorelli |
Thank you for contributing! This PR is in our backlog to review when someone has some time to look at it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just looked at the code change, haven't done any testing.
The tests are passing and adding the extra condition seems to make the logic stronger, so I'd say it's ok from my perspective.
…sioned methods. This resolves an issue with some modules that rely upon get_draft_site_secured, like Elemental and sharedraftcontent
7065eb2
to
7128123
Compare
Thank you, @michalkleiner. I just pushed up an update with your suggested alteration. |
Hi @nathanbrauer,
Probably, I need to do additional steps. Could you, please, provide steps that helps me to see problem. Tested in CMS 4.13 Thank you in advance. |
@sabina-talipova This seems to specifically be a problem with relations - the example in both the PR description and the original issue mention using elemental blocks. Please try adding some elemental blocks to the blog page and see if that helps you reproduce the issue. |
<?php
namespace {
use DNADesign\Elemental\Extensions\ElementalPageExtension;
use DNADesign\Elemental\Models\ElementalArea;
use SilverStripe\Blog\Model\BlogPost;
class MyBlogPage extends BlogPost
{
private static $extensions = [
ElementalPageExtension::class,
];
}
}
Screen.Recording.2023-09-21.at.2.03.36.PM.mov |
I'm a bit confused as to exactly how you have your environment set up. I'm unable to reproduce your results. I stripped down one of our repositories to demonstrate/reproduce the problem and solution here: Kazam_screencast_00000_150_compressed_mid.webmI've applied this pull request's patch to two entirely different projects already successfully (different SS and PHP versions for each). |
@nathanbrauer, thank you for your help and patience. I had to delete and restart the container with the database. I see the problem. The solution works great. |
@nathanbrauer, thank you for your contribution. Nice solution! |
This resolves a nuanced issue with some modules that rely upon get_draft_site_secured for draft previewing.
For example, without this fix, on an app with dnadesign/silverstripe-elemental and silverstripe/silverstripe-sharedraftcontent, a preview of a draft blog whose PublishDate is null or in the past works as expected, but if the PublishDate is in the future, none of the elemental elements will be visible in the preview. An update to BlogPostFilter is the only way to resolve the issue, and this fix serves to do just that.
This can be accepted into the
3.*
base as a bugfix then safely merged into4.*
as well.Editing my source branch is enabled for maintainers.
Fixes:
Attn: @muskie9