WordPoints / wordpoints

Points plugin for WordPress
GNU General Public License v2.0
20 stars 15 forks source link

Post term entities #461

Closed JDGrimes closed 7 years ago

JDGrimes commented 8 years ago

Previously https://github.com/WordPoints/hooks-api/issues/13, which I've left open for now since there is some code for this in that repo. The ultimate goal of that ticket is to support hook conditions of the type "post has term", but that requires additional work.

JDGrimes commented 7 years ago

One thing that I am wondering as I am copying this code over, is whether the storage info returned for the term entity should include a condition to only return terms for the particular taxonomy that the entity object is for. It seems like maybe it should, but it doesn't. The same would also go for the post entity; it should probably only include that particular post type. But I'll deal with that in a separate issue once I have copied these over.

JDGrimes commented 7 years ago

The only entity child for the term entities that we had there was the term ID, which really isn't needed (we don't have the ID attribute registered for any of the entities currently in core). Instead, we probably want to register attributes for the term name, and possibly the term slug. Terms can also now have metadata, but this is currently unused in WordPress core.

JDGrimes commented 7 years ago

As far as registering the slug as an attribute goes, we don't register an attribute for the post slug, so that may be unnecessary. It is more of a utility thing, not a user-facing attribute.

JDGrimes commented 7 years ago

There weren't any tests for the term entity classes though, so we'll need to add them, I guess.

JDGrimes commented 7 years ago

Adding tests for the post terms relationship reveals a problem. The primary and secondary entity slugs in the relationship need to be independently dynamic. Currently though, our dynamic relationship class makes both of them dynamic based on the same info from the slug. But we need the post entity slug to be set correctly with the dynamic post type slug, not the taxonomy slug, which is what currently happens. However, there is no way to know the post type slug currently, since only the taxonomy slug is included in the term entity slug. So perhaps we'll have to make it doubly dynamic.

Actually though, we can just use the parent slug, since it will be the slug of the post already. Actually, I guess that it is silly that we even modify the primary entity slug at all in the dynamic relationship class, since we will always want it to come out the same as the parent slug. So we can just use the parent slug directly and not worry about it. It just happens to be that in every case we've used dynamic relationships so far (post parent, comment post, and comment parent) the generated slug would be the same as the parent slug, since the dynamic portion of the parent and child are expected to be the same. This is the first case where that isn't expected to be so.

JDGrimes commented 7 years ago

Further investigation has revealed that the tests are actually messed up. The parent slug is not passed to the entity children when they are constructed, which is contributing to our problems.

JDGrimes commented 7 years ago

We still have to handle the fact that the dynamic portion of the related entity slug needs to be derived from the relationship slug, not the parent slug, as it now is since 2.3.0. Perhaps we can make this optional, so that if the relationship slug is dynamic, that will be used, but if it isn't, the parent slug will be used. I don't think that would break any of our dynamic relationships in core, as they set the relationship slug to have the same dynamic portion as the parent slug anyway. And I doubt that it would break anything else either. Really, it would be returning closer to the pre-2.3.0 behavior, so only anything built in the interim would possibly be expecting anything different. But since the dynamic portion of the relationship slug would just have been ignored, there would have been no reason for it to be set at all. So we should be good here.

JDGrimes commented 7 years ago

In addition to the term name, the term description, count, and parent (for hierarchical taxonomies), are stored in the term taxonomies table. These have a one-to-one correspondence to the terms now that terms can no longer be shared across taxonomies, so it is safe to treat these as foreign attributes of a term object. We should add entity attribute classes for them as well.

JDGrimes commented 7 years ago

I suppose for these we will actually a new kind of db storage type. We have field, and meta_table, but for this we just need table. I've updated the attribute storage info docs.

JDGrimes commented 7 years ago

The ultimate goal of that ticket is to support hook conditions of the type "post has term", but that requires additional work.

Actually, it would be sufficient. What we were thinking of was the fact that we really wanted to be able to have the query API, so that we could use it to do an autocomplete on the term. However, that really isn't a necessity, since the user can just enter the term name manually.

JDGrimes commented 7 years ago

We've come across a few issues while testing the conditions out though:

I'll open new tickets for each of these.

JDGrimes commented 7 years ago

The condition needs to be on the term name, I think, but can also be placed on the term itself, which I think would have to use the ID. Probably this would be confusing to users.

This still needs to be addressed. I'm wondering if it is actually a bug though, as we don't support the entity equals condition on Posts or Users, for example. It only appears to be supported within the entity array contains condition. The Role equals condition is also supported there, but of course that is an enumerable entity, so that is expected. I'll open an issue for it.

We also haven't addressed this:

One thing that I am wondering as I am copying this code over, is whether the storage info returned for the term entity should include a condition to only return terms for the particular taxonomy that the entity object is for. It seems like maybe it should, but it doesn't. The same would also go for the post entity; it should probably only include that particular post type. But I'll deal with that in a separate issue once I have copied these over.

New issues will need to be opened for that as well.

JDGrimes commented 7 years ago

And I just noticed one more issue: "Parent Term" is shown instead of "Parent Category" in the arg selector dropdown.