[D8] Quick Node Clone

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxvilepickle2636000git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2646958

Project 2: https://www.drupal.org/node/2636002

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

I'm a robot and this is an automated message from Project Applications Scraper.

Hi David,

I have reviewed your code and have next remarks:

1. In QuickNodeCloneNodeForm methods __construct() and form() only call parent ones. This is not required and these methods can be dropped
2. Please remove unused
   

       $node = $this->entity;
   

   from QuickNodeCloneNodeForm::actions() method

3. Change t() to $this->t() in QuickNodeCloneNodeForm class
4. Drop __construct() method from QuickNodeCloneEntityFormBuilder class as it is equal to parent one

5. public function clonePageTitle($node) {
       $parent  = Node::load($node);
   

   Remove extra space here.

6. class QuickNodeCloneNodeController extends NodeController implements ContainerInjectionInterface {
   

   implements ContainerInjectionInterface is not required because NodeController already implements this interface

Other looks good for me.
Thanks for your work!

tldr; From a general point of view, it looks like quite a few bits of this are copy/pasted and not updated correctly, e.g. use statements, documentation etc. Also, it isn't clear what makes this distinct enough from Node Clone to warrant a separate module.

I hope this doesn't come across as harsh, I have tried to be thorough and spot any potential issues that may hinder you application progressing.

Automated Review

There are a number of issues found by pareview.sh - http://pareview.sh/pareview/httpgitdrupalorgsandboxvilepickle2636000git

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

No: Causes module duplication and/or fragmentation. This appears to be the same functionality as Node Clone which has a D8 branch in development. Perhaps it would be better to work with them in getting a D8 release? You say in your summary

Written differently enough from node_clone to warrant a separate module.

Could you elaborate on what is different that warrants a separate module?

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template. Note PA review flags up line length which needs resolving, but the content is fine.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

No: List of security issues identified.

1. May be wrong, but \Drupal\quick_node_clone\Form\QuickNodeCloneNodeForm::actions() has $element['submit']['#access'] = TRUE; without any apparent checks on whether the user should have access.

Coding style & Drupal API usage

1. (*) The documentation and method do not match:
   

     /**
      * The _title_callback for the node.add route.
      *
      * @param \Drupal\node\NodeTypeInterface $node_type
      *   The current node.
      *
      * @return string
      *   The page title.
      */
     public function clonePageTitle($node) {
       $parent  = Node::load($node);
       return $this->t('Clone of "@node_id"', array(
         '@node_id' => $parent->getTitle()
         )
       );
     }
   

   What is $node, as if it is a node, it doesn't need loading, if it's a node type, Node::load() is invalid. My guess is it's a node id? In which case $nid would be more suitable (with appropriate documentation update) and $parent can become $node

2. (*) \Drupal\quick_node_clone\Form\QuickNodeCloneNodeForm::form() just calls it's parent and is unnecessary.
3. (*) \Drupal\quick_node_clone\Form\QuickNodeCloneNodeForm::actions() has the following:
   

       $element['submit']['#access'] = TRUE;
       $element['submit']['#value'] = $this->t('Save New Clone');
   
       if ($element['submit']['#access'] && \Drupal::currentUser()->hasPermission('access content overview')) {

   You have set #access to TRUE and then check whether it's TRUE. Also, are you sure you want to be settings #access straight to TRUE?

4. (*) Drupal 8 coding standard is to use [...] rather than array(...) (e.g. \Drupal\quick_node_clone\Controller\QuickNodeCloneNodeController::clonePageTitle)
5. (*) \Drupal\quick_node_clone\Form\QuickNodeCloneNodeForm::submitForm() is identical to it's parent.
6. (+) \Drupal\quick_node_clone\Form\QuickNodeCloneNodeForm::save() looks to be unnecessary as it's parent already deals with it all except for adding '(clone)' to the messages. Is that message necessary and worth duplicating so much code?
7. (+) \Drupal\quick_node_clone\Form\QuickNodeCloneNodeForm::actions() seems to duplicate quite a bit of it's parent, are you able to just alter the return rather than re-do the work?
8. (+) src/Controller/QuickNodeCloneNodeController.php The following use statements aren't ever used:
   

   use Drupal\Component\Utility\Xss;
   use Drupal\Core\Controller\ControllerBase;
   use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
   use Drupal\Core\Language\LanguageInterface;
   use Drupal\Core\Url;
   use Drupal\node\NodeTypeInterface;
   use Drupal\node\NodeInterface;
   

9. (+) src/Form/QuickNodeCloneNodeForm.php The following use statements aren't ever used:
   

   use Drupal\node\Entity\Node;
   use Drupal\Core\Entity\ContentEntityForm;
   use Drupal\Core\Entity\EntityManagerInterface;
   use Drupal\user\PrivateTempStoreFactory;
   use Drupal\Core\Routing\RouteMatch;
   use Symfony\Component\DependencyInjection\ContainerInterface;
   

10. (+) src/Entity/QuickNodeCloneEntityFormBuilder.php The following use statements aren't ever used:
    

    use Drupal\Core\Form\FormBuilderInterface;
    use Drupal\Core\Entity\EntityManagerInterface;
    

11. In your routing, I'm confused by the options.parameters - it seems to be referencing node types when that route is dealing with nodes. Is this a copy/paste hangover?
12. Are src/Entity/QuickNodeCloneEntityFormBuilder.php and src/Form/QuickNodeCloneNodeForm.php needed? It appears most of what exists in there could be dealt with in the controller, which would simplify the code considerably.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

This review uses the Project Application Review Template.

We, too, have found this module to be particularly useful. We have not found another module in D8 to replace the old D7 node_clone module. Like many other organizations, we dislike using modules not covered by the Drupal security policy.

I would like to see this get covered, use it in a production site and was the only module in d8 that worked for me and was super quick to install and get going.

The project link and the Git instructions need to be updated: The project link returns a 404 error.

See the errors reported from PAReview.

If you are still working on this application, you should fix all known problems and set the status to Needs review. (See also the project application workflow.)
Please don't change status of this application if you aren't sure you have time to dedicate to this application, or it will be closed again as won't fix.

I am closing this application due to lack of activity.

Problems have been fixed: https://www.drupal.org/project/quick_node_clone/issues/2978890

Setting this to Needs review.

Thanks to the module maintainer and also to d.o webmasters for their hard work!

PAReview reports no issues now 👍
https://pareview.sh/pareview/https-git.drupal.org-project-quick_node_clo...

Manual review

Individual user account
[Yes: Follows] the guidelines for individual user accounts.
No duplication
[Yes: Does not cause] module duplication and/or fragmentation.
Master Branch
[Yes: Follows] the guidelines for master branch.
Licensing
[Yes: Follows] the licensing requirements.
3rd party assets/code
[Yes: Follows] the guidelines for 3rd party assets/code.
README.txt/README.md
[No: Does not follow] the guidelines for in-project documentation and/or the README Template.
Missing documentation guide
 It's recommended to have an install hook. I've added the readme content to the Drupal documentation guides list: https://www.drupal.org/docs/8/modules/quick-node-clone it just needs to linked into the module page
Code long/complex enough for review
[Yes: Follows] the guidelines for project length and complexity.
Secure code
[Yes: Meets the security requirements.]
Coding style & Drupal API usage
Missing return tag in comment for QuickNodeCloneNodeAccess::cloneNode()
Random asterisk in comments for AddEventSubscriber class comment 'this*'


Thanks for the review and the feedback.

We've added a link to the documentation from the module homepage, and fixed the coding style issues you highlighted in your last review.
I believe we've now addressed everything that was preventing this from being approved, and would be grateful for a re-check. Many thanks.

With regards to linking to the documentation, please see https://www.drupal.org/project/quick_node_clone/issues/3090140

Thanks Nicrodgers. I've reviewed the changes and the issues are now resolved.

• What follows is a quick review of the project; it doesn't mean to be complete
• For every point, I didn't make a complete list of where the code should be fixed, but an example of what is wrong in the code
• Not all the points are application stoppers; some of them describe changes that would be preferable to make

quick_node_clone.paragraph_settings_form:
  path: '/admin/config/quick-node-clone-paragraph'
  defaults:
    _form: '\Drupal\quick_node_clone\Form\QuickNodeCloneParagraphSettingsForm'
    _title: 'Quick Node Paragraph Clone Setting'
  requirements:
    _permission: 'Administer Quick Node Clone Settings'

Since that route is for the settings of an entity defined from a third-party module this module doesn't require, it should only accessible when that module is installed.

/**
 * Returns responses for Quick Node Clone Node routes.
 */
class QuickNodeCloneNodeController extends NodeController {

Controllers are internal API and they should not be used as base classes. See Drupal 8 backwards compatibility and internal API policy (backend).

@internal

The following parts of the code base should always be treated as internal and are not guaranteed to not change between minor versions. For patch versions we will aim to avoid any breaking changes even to @internal code unless required to fix a serious bug:

# The database schema for core modules

While the database query API and schema API itself are stable, the schema of any particular table is not. Writing queries directly against core tables is not recommended and may result in code that breaks between minor versions.

# HTTP headers

The particular HTTP headers that core sends should not be considered an API. Neither interactions with headers via PHP (set through #attached), nor reverse proxy configurations. Specific HTTP headers may be relied upon, but this will usually be documented either in settings.php or inline.

# Automated test classes

• The contents of individual automated test classes provided by core modules are never considered API. Contributed and custom modules should not directly extend individual test classes as they may change at any time.
• Test traits and abstract base classes should generally use deprecation where possible rather than breaking backwards compatibility, but they are still considered internal API and may change if necessary.
• The testing framework itself (Simpletest, BrowserTestBase, etc.) is considered public API and should provide backwards compatibility between minor releases.

# Controllers and Form classes

Core modules contain many route controllers that are bound to individual routes, as well as form classes. These controllers are not part of the API of the module unless specifically marked with an @api tag on the method or class.

# Plugins

Particular plugins, whether class based or yaml based, should not be considered part of the public API. References to plugin IDs and settings in default configuration can be relied upon however.

# Entity handlers

Particular entity handlers should not be considered part of the public API. The interfaces which define those entity handlers though are part of the supported API.

# Paramconverters, Access checkers, Event subscribers, hook implementations etc.

Paramconverters, access checkers, event subscribers and similar services which are never expected to be used directly either as services or value objects, are not considered part of the API. You should not extend from these classes and provide your own implementation instead.

  public function clonePageTitle(Node $node) {
    $prepend_text = "";
    $config = \Drupal::config('quick_node_clone.settings');
    if (!empty($config->get('text_to_prepend_to_title'))) {
      $prepend_text = $config->get('text_to_prepend_to_title') . " ";
    }
    return $prepend_text . $node->getTitle();
  }

A controller class should not use the \Drupal class as it can inject its dependencies in the create() method.

class QuickNodeCloneNodeFinder {

Service classes should implement an interface.

  /**
   * QuickNodeCloneNodeFinder constructor.
   *
   * @param \Symfony\Component\HttpFoundation\RequestStack $requestStack
   *   Request stack.
   * @param \Drupal\Core\Path\AliasManager $aliasManager
   *   Alias manager.
   * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entityTypeManager
   *   The entity type manager.
   */
  public function __construct(RequestStack $requestStack, AliasManager $aliasManager, EntityTypeManagerInterface $entityTypeManager) {
    $this->requestStack = $requestStack;
    $this->aliasManager = $aliasManager;
    $this->entityTypeManager = $entityTypeManager;
  }

The parameter types should be interfaces, when they exist.

These applications aren't for projects, but for users, since we don't alter projects but assign a new role to the user who created the application. As such, it's the user who applied that needs to change status to Needs review, when what reported as been fixed.

Hi @kiamlaluno, thanks for your feedback.

You say:

Not all the points are application stoppers; some of them describe changes that would be preferable to make

Can you clarify which point(s) are application stoppers, so that we can prioritise addressing those initially?

In this case, creating an interface used for the QuickNodeCloneNodeFinder class is not a strictly necessary change, even if it would preferable for module that override this module.

This is a question that should ask the user who opened this application, though, as this is an application to give him a Drupal role, not an application to change settings for a project.

These applications are not done for projects, but users.
We aren't giving projects any special status; we are simply giving to users the possibility to opt into security coverage for projects they created or they will create, byt assigning to their accounts a new Drupal role. As such, how much sites use the application project is not a parameter we consider, since we just use the project to understand how much the user (not a group of users) who applied understands of writing secure code that correctly uses the Drupal API and follows the coding standards.

If you aren't interested in being able to opt into security coverage for projects you created or you will create, we can close this application.

Nobody else can use https://www.drupal.org/project/quick_node_clone to be able to opt into security coverage for their projects, but this isn't even necessary, since the project used for these applications is not necessarily the project for which somebody would want to opt into security coverage.
If I didn't have the vetted role, and I were the main maintainer of a project with more commits from another user, I would need to create a project with commits only from me, and use that project to show what I understand in writing Drupal code. Once my application were approved, I would be able to opt into security coverage for any of the projects I maintain, including the ones for which I didn't write most of the code.

These applications are about the user who applies and giving that user a user role after reviewing code that user wrote. It's not about other users, or the collective work they do.

Since this is yours application, other users cannot tell the reviewers we have fixed it and change the status to Needs review since that is the task for the user who applies.

Thanks for you contribution!

1. QuickNodeCloneNodeAccess should not rely on the global user but should pass on the received $account object for which to check access. I cannot think of an attack that would result in some privilege escalation since this class is used only for one route's access control, but this should be fixed anyway.
2. QuickNodeCloneNodeController::clonePageTitle(): doc block is wrong, this is not node.add.

Otherwise looks good to me!

I can see that vilepickle has lots of commits on the projects, has dutifully merged and reviewed changes from other contributors. Therefore we can assume they have the understanding of Drupal modules that kiamlaluno was looking for.

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 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.