Plugin code review?

So Erin,

I had a look at this today. And I’m a little confused. What exactly are you trying to do with the plugin?

I have this in a template:

{exp:gallery_categoree parse="inwards"}
    cat is {cat_id}
    {exp:gallery:entries gallery="{gallery_name}" category="{cat_id}" orderby="random" columns="1" rows="1"}
        {image_url}
    {/exp:gallery:entries}
{/exp:gallery_categoree}

I see that it retrieves the cat_id from the URL. But from there all that happens is it outputs cat is {cat_id}. And then the gallery entries tag underneath just gets cleaned out from what I can tell…

Can I get a clear description of where you would use this, how, and why? I can probably help you out a bit more then. 😊

Jamie

Thanks for looking into this Jamie. I think you are looking at the original version, not the version I attached a few days ago.

This version takes in a gallery parameter and based on the URL segment (either C1, etc or Cat name), returns a few category properties (total_files, num_sub_cats, cat_name, cat_id) and the list of sub-categories in a tag pair.

Excuse the usage part as it isn’t right.

Thanks

Erin,

Ok, I’m understanding it now I think. You only have one required parameter (gallery). And from that and the URL segment you find the required information.

A few things after looking through it all.

The exp_gallery_categories includes a gallery_id column. That means you know the gallery you are working with from the category. So the gallery parameter could be unnecessary.

You currently have three queries:

One to get the gallery_id from the gallery parameter.

Then one to grab the category we are looking at.

Then one to grab any child categories if they exist.

You could probably cut that down to two. One to get the gallery_id from the cat_id being viewed. Then one to grab all of the categories assigned to that gallery.

From the second query you could build an array with the info you need. The info for the category being viewed and then by comparing the rest of the categories grabbed you can determine any children if they exist.

Also in two of your queries you are joining two tables:

FROM exp_gallery_categories as c, exp_galleries as g

But you never query for or compare against anything in the exp_galleries table. There is no reason to increase the complexity of your query like that if you don’t actually need anything in the exp_galleries table.

However, as it is, it does work. And it all seems to work like you would expect.

One other thought. If this plugin were designed to occur within one of the already existing gallery tags on a gallery page you could feed it the galery_id directly and then eliminate another of the queries and bring them down to just the one to grab the category data.

Also as a test case I’m using this:

{exp:gallery_categoree gallery="gallery"}
    Category Name: {cat_name}

    Category ID: {cat_id}

    Number of Sub Categories: {num_sub_cats}

    Total Files: {total_files}
    {if num_sub_cats != 0}
    
Sub Categories:

        {sub_cats}
                 Sub Category Name: {sub_cat_name}

                 Sub Category ID: {sub_cat_id}

        {/sub_cats}
    {/if}
{/exp:gallery_categoree}

That is going to do everything it’s capable of correct?

Jamie

First off, I really, really appreciate this Jamie. Getting off on the correct foot will make it much easier for me to contribute and build more functionality.

Ideally (and at a later time), I would like to extend the PhotoGallery module to add this functionality as well as a few other that I have in mind (like category images). But for now I just want to get my photo gallery up.

The exp_gallery_categories includes a gallery_id column. That means you know the gallery you are working with from the category. So the gallery parameter could be unnecessary.

I thought this too. Then I thought, “well, why does the gallery module need it? If they need it, maybe there is a reason”. Also, can Categories be duplicated (by name) across galleries? If so, then you do need to pass in the gallery name/id.

You could probably cut that down to two. One to get the gallery_id from the cat_id being viewed. Then one to grab all of the categories assigned to that gallery. From the second query you could build an array with the info you need. The info for the category being viewed and then by comparing the rest of the categories grabbed you can determine any children if they exist.

So the first one would be something like “select gallery_id from exp_g_cat where cat_id is {cat_id}”.

The second confuses me a little. How do I get aggregate data (the sub-cats) and single data (total_files) in the same query. My SQL-fu is not up to coming up with a quick answer.

But you never query for or compare against anything in the exp_galleries table. There is no reason to increase the complexity of your query like that if you don’t actually need anything in the exp_galleries table.

True, I was doing that just in case I duplicate Category names. But as I am only using one gallery, and I haven’t decided whether to release this yet (though publishing on the forums “releases” it I suppose), I guess I can get get rid of that.

One other thought. If this plugin were designed to occur within one of the already existing gallery tags on a gallery page you could feed it the galery_id directly and then eliminate another of the queries and bring them down to just the one to grab the category data.

Yes, but I need the info from this plugin to pass to the gallery tag, namely the category. Or am I misunderstanding you?

Also as a test case I’m using this:

{exp:gallery_categoree gallery="gallery"}
    Category Name: {cat_name}

    Category ID: {cat_id}

    Number of Sub Categories: {num_sub_cats}

    Total Files: {total_files}
    {if num_sub_cats != 0}
    
Sub Categories:

        {sub_cats}
                 Sub Category Name: {sub_cat_name}

                 Sub Category ID: {sub_cat_id}

        {/sub_cats}
    {/if}
{/exp:gallery_categoree}

That is going to do everything it’s capable of correct?

Yup, you got it.

Thanks again for your thoughts Jamie. From what I can tell, there aren’t any glaring errors or no-nos, just some non-optimal querying, so that is good to know.

Erin,

You nailed it on the multiple galleries thing regarding why to require a gallery parameter. Not sure why that didn’t occur to me. But yes. You could have a gallery category with the same name in two different galleries and that would certainly cause an issue if you didn’t have a gallery parameter to limit the query to grab the categories.

On the second query you have two choices. Just grab all of the categories for a gallery. (Not a problem if there aren’t thousands). Then you parse through it on the PHP end.

Or if you wanted to be a bit more efficient something like this would work:

SELECT cat_id, parent_id, total_files, cat_name
FROM exp_gallery_categories
WHERE gallery_id = '1'
AND (
    cat_id = '2'
    OR parent_id = '2'
    )

Then you can check the $row[‘parent_id’] result to see which one is your category and which ones are children of it.

Jamie

EDIT: Accidentally hit submit before I was done typing.

Yes, but I need the info from this plugin to pass to the gallery tag, namely the category. Or am I misunderstanding you?

My turn not to understand. Why would you need to pass the plugin info to the gallery tag? It doesn’t need to know any of that to properly work does it?

I guess that gets back to my first question of how you intend to use this?

Jamie

Ah, nifty!

Thanks for your thoughts.

What I am hoping is that the EE crew implement all of this in v2 so that I don’t have to go through the effort to extend the Gallery module and add a bunch of functionality!

I will make a few efficiency adjustments and call it done!

Thanks a bunch Jamie!

Yes, but I need the info from this plugin to pass to the gallery tag, namely the category. Or am I misunderstanding you?

My turn not to understand. Why would you need to pass the plugin info to the gallery tag? It doesn’t need to know any of that to properly work does it?

I added the ability to use the Category name instead of the always confusing C2, etc in the URL. SO I need to figure out the cat_id before I can call the gallery tag.

Now, if the EE crew added this capability coughfeaturerequestcough, then you are right, I could put it inside a gallery tag.

For the moment though, it has to be outside it.

Thanks

Oh, OK. I get it.

Jamie