[1.0.x] Moderation State Condition

Thank you for applying!

Please read Review process for security advisory coverage: What to expect → for more details and Security advisory coverage application checklist → to understand what reviewers look for. Tips for ensuring a smooth review → gives some hints for a smoother review.

The important notes are the following.

• If you have not done it yet, you should run phpcs --standard=Drupal,DrupalPractice on the project, which alone fixes most of what reviewers would report.
• For the time this application is open, only your commits are allowed.
• The purpose of this application is giving you a new drupal.org role that allows you to opt projects into security advisory coverage, either projects you already created, or projects you will create. The project status won't be changed by this application and no other user will be able to opt projects into security advisory policy.
• We only accept an application per user. If you change your mind about the project to use for this application, or it is necessary to use a different project for the application, please update the issue summary with the link to the correct project and the issue title with the project name and the branch to review.

To the reviewers

Please read How to review security advisory coverage applications → , Application workflow → , What to cover in an application review → , and Tools to use for reviews → .

The important notes are the following.

• It is preferable to wait for a Code Review Administrator before commenting on newly created applications. Code Review Administrators will do some preliminary checks that are necessary before any change on the project files is suggested.
• Reviewers should show the output of a CLI tool → only once per application.
• It may be best to have the applicant fix things before further review.

For new reviewers, I would also suggest to first read In which way the issue queue for coverage applications is different from other project queues → .

Usually, after reviewing a project, we allow the developer to opt projects into security advisory coverage. This project is too small for us and it doesn't contain enough PHP code to really assess your skills as developer.

Have you created any other project on drupal.org (module, theme, distribution) we could instead review? The project needs to have most of the commits (preferable all the commits) done by you.

I also created Group Computed Field ( https://www.drupal.org/project/group_computed_field → )

My co-maintainer already opted it in to security coverage, but I wrote pretty much all the code for it. I wasn't sure if it would count since it was already opted in.

That all being said, I don't know if it has enough PHP in it either- it's a pretty small module.

If it would be a better candidate I can make a new application or alter this one if that works.

Yes, the other project will be better. The fact the project is already covered by the security advisory policy does not matter, as this application is to give you the permission to opt projects into security advisory coverage.

If you want to use that module, just leave a comment here. I will edit the issue summary.

Ok, cool. Let's use that one then!

Thank you again for applying and for your prompt reply! We are now ready for the first review.

• What follows is a quick review of the project; it doesn't mean to be complete
• For each point, the review usually shows some lines that should be fixed (except in the case the point is about the full content of a file); it doesn't show all the lines that need to be changed for the same reason
• A review is about code that doesn't follow the coding standards, contains possible security issue, or doesn't correctly use the Drupal API; the single points aren't ordered, not even by importance

src/Plugin/Field/RelatedGroupFieldItemList.php

As per Drupal core backend backwards compatibility and internal API policy (Drupal 8 and above) → , plugins are not public API, even if they are not marked with @internal. This means that contributed projects cannot extend a core class to implement a plugin; the only exception is for base classes, but their names end with Base.

group_computed_field.install

hook_install() implementations are invoked when a module is installed. The documentation comment for group_computed_field_install() seems to assume it is invoked also when the module is updated, which is not true.
In this case, it is not necessary to implement hook_install() because the field will be added to entities by invoking group_computed_field_entity_base_field_info(). See the content of content_moderation.install which does not contain a hook_install() implementation, despite the module adds a computed field in content_moderation_entity_base_field_info(). (I am using links to Drupal 9 documentation pages simply because some documentation pages are not shown in the Drupal 10 documentation.)

hook_uninstall() is not necessary too: Drupal core remove those fields that are added/implemented by modules which are uninstalled. Differently, the content_moderation.install file previously mentioned would contain a
hook_uninstall() implementation.

For src/Plugin/Field/RelatedGroupFieldItemList.php:

Are you referring to me extending FieldItemList? I was basing this off of core modules (like content_moderation) where the ModerationStateFieldItemList extends FieldItemList.

Would I need to make my own base class that extends FieldItemList and then use that in the plugin?

For group_computed_field.install

I have removed the install hook entirely and removed the definition removal from the uninstall hook, however I left in the functionality that removes "related_groups" from entity_view_display configs. In my testing, uninstalling the module does not automatically remove the field from the configs that used it.

Drupal core backend backwards compatibility and internal API policy (Drupal 8 and above) is for contributed projects that use the Drupal API, including the API implemented by Drupal core modules. In fact, that page includes sections like The database schema for core modules and Paramconverters, Access checkers, Event subscribers, hook implementations etc.

In this case, to avoid to use FieldItemList, you should create a class with almost all the code that class has (except in the case some of those methods are not necessary for you). (There is not a base class that could be instead used, as in other cases.)
If you are going to use that class, keep in mind that any change to that class are done in a way that could not preserve BC, with all the consequences of the case.

I mean sure, I can copy paste the class... But there are all sorts of traps there are there not? Creating a brand new copy of FieldItemList just for the sake of not using the original class would open up a whole new can of worms if anything tries to check if my list is a FieldItemList. Kind of defeats the point of object oriented coding.

If it's what I need to do I can do it, but that feels really wrong.

It's tricky in both the cases.
In the first case, though, you are only required to create a class that implements the same interface implemented by FieldItemList; it would not be necessary to change the code of the new class every time FieldItemList's code is changed. In the second case, you risk that a change in that class creates issues in your module.

Either ways are fine, as long as you understand the risk of using a class (or extending a class) that is considered internal.

To make it clear: I am not saying that a new class must be implemented. I can understand there are cases where following what reported by the backend backwards compatibility policy is not possible, for example avoiding to use a public property of a Drupal core class which does not have get/set methods for that property, when public class properties are considered internal API.

Public properties on a class are always considered @internal, this does not include the use of __get() and __set() to provide access to protected properties.

If you decide not to implement a custom class, you can set this application to Needs review, once the files are changed for the other review points.

Ok cool, thanks for clarifying!

In this case I think extending the class, while understand the risks of doing so, is the right way to go.

I have made the other suggested changes to the 3.x branch, so this is now ready for review again.

Thank you for your contribution! I am going to update your account.

These are some recommended readings to help with excellent maintainership:

• Dries → ' post on Responsible maintainers
• Best practices for creating and maintaining projects →
• Maintaining a drupal.org project with Git →
• Commit messages - providing history and credit →
• Release naming conventions → .
• Helping maintainers in the issue queues →

You can find more contributors chatting on the Slack → #contribute channel. So, come hang out and stay involved → .
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review → . I encourage you to learn more about that process and join the group of reviewers.

I thank all the reviewers.

Thank you! I appreciate the feedback and your time!

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