RadoslavGeorgiev / rila-framework

A front-end WordPress framework with a set of many extendable class wrappers, inpired by the MVC ideology
GNU General Public License v2.0
18 stars 3 forks source link

Allow to pass an array to get the first hit in a get_posts query #10

Closed JuhG closed 7 years ago

RadoslavGeorgiev commented 7 years ago

Hi @JuhG,

I don't completely approve this implementation for a few reasons:

  1. When/if there are no posts that match the given criteria in the database, the function will throw a Missing_Object_Exception. Even though this could be handled externally, many people will forget to do it properly and would get an ugly exception if, let's say, a page gets deleted. It would be cooler if the function can handle the following usage: if( $post = rila_post( $args ) ) { ... }.
  2. Currently the framework mostly read-only, but I want to extend the built-in objects in order to allow the creation of items. In that case, the parameter you are using would receive an array of post data.
  3. This code will only work with posts. It would be much cooler if you can do the same for each main object type.
  4. Generally, unless the post is already "selected" (you have at least the ID of the post), it's a better idea to use collections. You can see how to implement this below.
$args = array(
    'post_type' => 'event'
);

$collection = new Rila\Posts( $args );
if( $post = $collection->at( 0 ) ) {
    // ...
}

// or

$collection = new Rila\Posts();

if( $post = $collection->type( 'event' )->at( 0 ) ) {   
    // ...
}

There are a few catches though:

  1. This code is too long. It would be a good idea to create function shortcuts to collections, for example rila_posts(), rila_terms() and etc. Then you can go ahead with if( $post = rila_posts()->type( 'event' )->at( 0 ) ) { ... }
  2. The at() method currently does not check if an element exists. We need to modify it to return false or null when there is no post.
  3. We could introduce first() and last() methods (along with others typical collection methods). Then we would be able to have if( $post = rila_posts()->type( 'event' )->first() ) { ... }.

One last concern would be the amount of items (posts in this case), which are being retrieved. first() could be implemented in a way that clones the original collection and sets the limit to 1 post, so we don't get an additional DB query just for that post. last() on the other hand, would also reverse the order.

What are your thoughts on this?

JuhG commented 7 years ago

The original idea was to add some extra functionality without hurting anything existing. When I know I need only one post if feels overwhelming to create a collection, but it's the best option right now.

When I needed the last item of a post type I instantly tried to write:

$last_event = rila_post(array(
    'post_type' => 'event'
));

Because that's what rila_post does, returns exactly one post.

You're right about the exception. If the returned array is empty, it should instantly return false or null. Otherwise I think it should work like above, right now it doesn't accept an array, so it doesn't conflict with anything, but maybe it's not intuitive enough. I accept that you don't like it.

I like both of your suggestions, rila_posts() and first() / last(). They are more in line with the framework. Maybe an extra suggestion to that, rila_posts could receive post type as a default argument.

RadoslavGeorgiev commented 7 years ago

After a couple of commits, you can actually use the shortcuts, so that the following code actually works:

rila_posts()->type( 'event ')->first()

Just FYI, this is something I had forgotten and it's the rila function. Its documentation is not ready yet, but it's a function that accepts a lot of argument types/"selectors". If it receives no arguments, it returns a new posts collection, so the example above can be changed to the following now:

rila()->type( 'event' )->first()