search

MITLibraries / MITLibraries-child

A Wordpress child theme that descends from MITLibraries-parent
GNU General Public License v3.0
1 stars 1 forks source link

Sticky posts are acting weird in front page template #59

Closed PBruk closed 8 years ago

PBruk commented 8 years ago

Tagging @frrrances for code-review: Checks for sticky posts, returns nothing otherwise. Resolves #54.

frrrances commented 8 years ago

Looks good but my php is rusty... tagging @matt-bernhardt for a second review.

matt-bernhardt commented 8 years ago

The changes to inc/content-front.php seem to introduce a syntax error that's being flagged by CodeSniffer - specifcially, there's what looks like the tail end of an if/else structure on lines 38-42, but I don't see it opening anywhere.

My suspicion is that if I deploy this branch, it will crash the front page of any site that uses that template.

My assumption is that there needs to be a WP_Query on this template that grabs all (and only) sticky posts for a loop?

matt-bernhardt commented 8 years ago

Some notes on this re-written branch...

The approach started by @PBruk was valid, assuming that WP_Query behaves in a sane way. Unfortunately, in this case that doesn't seem to be true.

The original approach was to generate an array of sticky posts using get_option( 'sticky_posts' ), and then sort and slice it to five items. That sorted array was then fed to a WP_Query object, which worked great so long as the array had any items. Unfortunately, it appears that WP_Query, when presented with an array of zero length, instead returns all items.

Because of this, I've refactored the page slightly, to check for the length of the array generated by get_option( 'sticky_posts' ). If that array has zero length, then the entire section gets skipped - which cuts down on a bit of computation and avoids the irrational need of WP_Query to return something.

With that, we're ready for a new code review by one or both of @PBruk and @frrrances

frrrances commented 8 years ago

Overall, makes sense to me, though i'll let @PBruk review the php details. I had one small comment at the end as it looked like the sidebar would be rendered twice.

matt-bernhardt commented 8 years ago

This is going to collide with the fix for #65, which has also made changes to content-front.php. I'm not sure which should merge first, but let's discuss tomorrow.

frrrances commented 8 years ago

👍