[D8] User Created by Field

Looking good! After reviewing this module's code and the code for the 'User Created by' module that you linked to, I agree that this is a different enough approach to warrant a separate module.

Code review:

• Following secure coding practices.
• Passes other PAReview standards.
• One recommendation: Remove the `templates` folder from your project directory, as the .twig file you have in that folder is empty.

Testing:

I ran into some issues while testing this on a fresh Drupal install (installed via Composer). The module installs well and works just fine, but on uninstall, I got some errors back. Here was the first one:

Error: Class 'FieldStorageConfig' not found in user_created_by_field_uninstall() (line 15 of /test_project/web/modules/contrib/user_created_by_field/user_created_by_field.install)

I was able to make this go away by adding the following 'use' statements at the beginning of the user_created_by_field.install file:

use Drupal\field\Entity\FieldStorageConfig;
use Drupal\field\Entity\FieldConfig;

However, I ran the uninstall again after that error and got back this message:

Error: Call to a member function delete() on null in user_created_by_field_uninstall() (line 21 of /test_project/web/modules/contrib/user_created_by_field/user_created_by_field.install)

The field appears to have uninstalled successfully as it no longer appears in the UI or the database, but this error should still be resolved. Note: The error above says line 21, but that is after adding the lines I mentioned before. It corresponds to line 18 in the current codebase.

Good work overall! Once you resolve that error, I sign off on this as reviewed.

    // if user does not have permission to edit, don't show field
    if(!$user->hasPermission('edit user created by field')) {
      unset($form['field_user_created_by_field']);
    }

Instead of unsetting a form element, the code should set the #access property for that form element or (even better) implement the field access hook not to give the edit access to that field to users who don't have the permission.

function user_created_by_field_preprocess_field__field_user_created_by_field(&$variables) {
  // Get the current user
  $user = \Drupal::currentUser();

  // Check for permission. if the user doesn't have it, hide the field
  if (!$user->hasPermission('view user created by field')) {
    unset($variables['label']);
    unset($variables['items']);
  }
}

There is no need of that code. It's enough to implement what I described.

The test isn't testing the module. Drupal core already tests the front page is accessible.

The uninstall problems have some additional side effects I had not previously mentioned. While the field will be removed from the user entity and the corresponding views, the module itself does not uninstall, meaning that the codebase cannot be removed or it will cause errors on other admin pages, such as when trying to access /admin/config.

I jumped on to review the changes. The code changes for the uninstall script look really good, but I am currently unable to access a lot of admin pages again. I am getting this error, which I believe is related to the changes in the .module file:

TypeError: Argument 2 passed to user_created_by_field_help() must be an instance of RouteMatchInterface, instance of Drupal\Core\Routing\CurrentRouteMatch given in user_created_by_field_help() line 16 of /test_project/web/modules/contrib/user_created_by_field/user_created_by_field.module #0 [internal function]: user_created_by_field_help('system.admin_re...', Object(Drupal\Core\Routing\CurrentRouteMatch))

I tried this on the existing site I had of the module and on a fresh install with the same results. I also tried downloading the module via Git and via Composer. This error is preventing uninstalling the module via the UI as I cannot access the Uninstall admin page.

I had posted the whole stack trace here but it was a bit lengthy and I'm not sure that you will need it. If you would like the entire stack trace, however, I have saved it so that I can provide it.

(I take the previous comment means the code has been fixed basing on what reported here.)

@eahonet, No need to apologize. :)

Good job fixing those issues! I approve this module and believe it is ready for security coverage.

Thank you for your contribution! I am going to update the project to opt it into security coverage.

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 IRC #drupal-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 dedicated reviewers as well.

You need to apply with a project that contains more code. A module with few code lines doesn't allow to verify what you understand about writing secure code that follows the coding standards and correctly uses the Drupal API.