GroupRelationshipTypeStorage::loadByEntity()

Before I reviewed that documentation page, I just assumed the method changed to GroupRelationship::loadByEntity() which interestingly does exist, but it doesn't seem to be working as expected for me. It's not returning any entities when there should be some.

So I'm not sure if the documentation is wrong, or if there's a bug in the new function, etc.

Update:

After further review, GroupRelationship::loadByEntity() is actually working correctly for me. So I assume that's the correct function to use, and it does actually use \Drupal::entityTypeManager()->getStorage('group_content')->loadByEntity() under the hood.

So I just think the documentation is not very clear when it says "Use GroupRelationshipTypeStorage class and it's loadByEntity method" because that lead me to mistakenly think I should replace GroupContent::loadByEntity() with GroupRelationshipTypeStorage::loadByEntity()

Methods on a class are often referred to as CLASS::method, but that doesn't necessarily mean they are static. Even though it looks like a static method, it' merely a widely adopted way of pointing to a method on a class.

The documentation correctly points out it's best to directly use the storage as the method on the GroupRelationship class is merely a wrapper and therefore (marginally) slower.

At that point you're supposed to look at the method and use it the right way:

• CLASS::method if it's static
• $class->method() if it's not

Seems like you figured it out on your own, though. But hopefully this helps.

Yes, I eventually figured it out. But I do feel this is a good example of how you could make your documentation clearer and easier to understand because technically the documentation is currently incorrect.

You have a section describing changes to the GroupContent/GroupRelationship class which says that the loadByEntity method was removed. This is incorrect because that method still exists and works just fine (and actually does what the documentation recommends doing instead).

Don't state that a method has been removed if it hasn't actually been removed. (And if it was actually removed and the old method was static but the new method is not static, then you should state that change in the documentation when you mention the new method's name so that we don't have to go hunting through the source code to figure it out.)

I would recommend just removing that entire table row from the documentation because loadByEntity was not removed so the user doesn't need to do anything other than use the new class name.

Don't state that a method has been removed if it hasn't actually been removed.

...

I would recommend just removing that entire table row from the documentation because loadByEntity was not removed so the user doesn't need to do anything other than use the new class name.

Fully agreed. That documentation was a community effort (as in, I didn't write it) so feel free to remove said row.

Regarding static vs non-static, I disagree. It's always up to the implementer to check the signature of the function they're implementing. But why didn't your IDE point that out? If I tried to call a regular method on a class as a static, PhpStorm would tell me about that. I'm sure other popular IDEs do too.

Ah, thanks, I didn't realize I could edit that documentation. I've removed that row and made a couple of other additions to improve it.

Regarding static vs non-static, no, my VS Code editor doesn't alert to to that issue. Perhaps I don't have it configured correctly. Just goes to show you can't assume everyone's using an IDE that would point that issue out. I guess I just feel that a section of documentation that is dedicated to listing API changes should state when a method changes from static to non-static because that's just as important of an API change as the method name changing or parameters changing.

Thanks for the update!

API changes should state when a method changes from static to non-static

Most definitely, as that would be a backwards compatibility break and require a new major version.

But that's not what happened here, GroupRelationship::loadByEntity() has always been static and GroupRelationshipStorage::loadByEntity() has always not been. Neither of them changed. The erroneous line in the documentation could have given you a false assumption, sure, but as I mentioned earlier you should always look up the method you're implementing, read its docs and signature and then implement it.

Blindly calling methods will lead to bugs, as you experienced for yourself.

Automatically closed - issue fixed for 2 weeks with no activity.