In my Drupal 8 Beta development training I have chosen to practice the upgrade process with the node_clone module. It will advance slowly in small steps.

There is a sandbox to keep track on my changes here.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

franskuipers’s picture

Issue summary: View changes
franskuipers’s picture

I upgraded the hook_menu into the new routing system.

There are 2 paths:

  • the admin form: admin/config/content/clone
  • the node clone route (doing noting yet) node/{node}/clone/{clone_token}

The original names of the file was clone.*. These are renamed to node_clone.* because I got an error at loading the CloneController class file.

Some observations:

  • it looks like menu entries with a callback of drupal_get_form in D7, will always being migrated with a Drupal\Core\Form\ class
  • module config settings are best handled by the class Drupal\Core\Form\ConfigFormBase.
  • module config settings are best handled by the class Drupal\Core\Form\ConfigFormBase.

Reviews are very welcome.

franskuipers’s picture

layout change

franskuipers’s picture

layout change

franskuipers’s picture

layout change

franskuipers’s picture

The big patch file....

95% is not migrated yet.

franskuipers’s picture

This file is easier to review with only the files I have touched till now.

Sutharsan’s picture

  1. +++ b/config/schema/node_clone.schema.yml
    @@ -0,0 +1,19 @@
    +globalredirect.settings:
    +  type: mapping
    +  label: 'Node Clone Settings'
    

    Copy/paste error: use "clone_method.settings" instead.

  2. +++ b/lib/Drupal/node_clone/Controller/CloneController.php
    @@ -0,0 +1,41 @@
    +  /**
    +   * Controller content callback: Hello page with dynamic URL
    +   *
    +   * @param \Drupal\node\NodeInterface $node
    

    Needs to be updated.

  3. +++ b/lib/Drupal/node_clone/Controller/CloneController.php
    @@ -0,0 +1,41 @@
    +   * @return string
    +   */
    +  public function cloneContent(NodeInterface $node, Request $request) {
    

    Returns a (render) array.

  4. +++ b/lib/Drupal/node_clone/Controller/CloneController.php
    @@ -0,0 +1,41 @@
    +    // Get the raw parameter from the URL.
    +    $nid = $request->attributes->get('_raw_variables')->get('node');
    +    $token = $request->attributes->get('_raw_variables')->get('clone_token');
    +    $output['hello'] = array(
    

    No need to get the raw variables here. $node contains the nid too ($node->getId()) and using $node_token in the controller method will give you the token value. (cloneContent(NodeInterface $node, $node_token)).

  5. +++ b/lib/Drupal/node_clone/Forms/CloneSettingsForm.php
    @@ -0,0 +1,117 @@
    +    $config = $this->config('node_clone.settings');
    +    $settings = $config->get();
    +
    

    Common usage is, for example:

    $config->get('clone_method');
    $config->get('clone_menu_links');
    
  6. +++ b/lib/Drupal/node_clone/Forms/CloneSettingsForm.php
    @@ -0,0 +1,117 @@
    +    $form['basic'] = array(
    +      '#type' => 'fieldset',
    +      '#title' => t('General settings'),
    

    Don't use t() in classes. Use $this->t in forms instead.

  7. +++ b/lib/Drupal/node_clone/Forms/CloneSettingsForm.php
    @@ -0,0 +1,117 @@
    +    $form['basic']['clone_menu_links'] = array(
    +      '#type' => 'radios',
    +      '#title' => t('Clone menu links'),
    +      '#options' => array(0 => t('No'), 1 => t('Yes')),
    +      '#default_value' => (int) $settings['clone_menu_links'],
    +      '#description' => t('Should any menu link for a node also be cloned?'),
    +    );
    

    Yes/No radios are discouraged. Use a checkbox instead.

  8. +++ b/lib/Drupal/node_clone/Forms/CloneSettingsForm.php
    @@ -0,0 +1,117 @@
    +    $config = $this->configFactory->get('node_clone.settings');
    +
    

    Use

    $this->config('node_clone.settings');
    
franskuipers’s picture

This is a working node_clone in the current 8.x-1.x branch of D8 core.

What is working:

  • routing
  • config
  • admin
  • clone nodes with prepopulate
  • clone menu links
  • reset publishing options

Looking forward to the reviews.
PS: the patch is the easy-to-review version.

pwolanin’s picture

Status: Active » Needs review

need to mark it as needing review

pwolanin’s picture

Status: Needs review » Needs work

The patch doesn't look like it applies on top of the 7.x version? The module file is fully added, for example.

I just pushed a 8.x-1.x branch that is a copy of 7.x-1.x

Please post a patch that applies to that branch.

franskuipers’s picture

Thanks for looking into this, Peter

No the patch was not against the 7.x branch, because then it is impossible (I think) to have a good review.

This patch is against your 8.x-1.x branch.

It is a working version of node_clone in drupal8 8.x branch. I admit there is still a lot to do, but this is a good starting point for further development. When this patch is in I would like to create a number of issues/todos against version 8.x-1.x. The patch reviews per issue will be easier.

BTW: I added a "git format-patch" file if you like to keep my individual commits.

franskuipers’s picture

Status: Needs work » Needs review

Needs review

Sutharsan’s picture

PSR-0 to PSR-4 changes applied (https://www.drupal.org/node/2246699).

@franskuipers, you are assigned to this issue. Are you still working on/reviewing this patch?

Sutharsan’s picture

Status: Needs review » Needs work
  • clone.views.inc, views_handler_field_node_link_clone.inc
    Remove D7 code and/or add @todo comments.
  • node_clone.module
    function node_clone_help($path, $arg) {
    ...
    //      $method = variable_get('clone_method', 'prepopulate');
    

    Replace remaining variable_get() calls.

  • node_clone.settings.yml
    clone_method: 'prepopulate'
    clone_nodes_without_confirm: '0'
    clone_menu_links: 0
    clone_use_node_type_name: 0
    

    These configurations only exist within the scope of node_clone. Minimise of modify the config vars. e.g. use node_clone.method instead of node_clone.clone_method.
    The D7 module has more variables. Missing in D8: clone_omitted and clone_reset_*. Should these be added?

  • NodeCloneForm.php
      public function form(array $form, array &$form_state) {
         ...
        // user should not unset 'Create new revision'
        // unset($form['revision']);
        // unset($form['log']);
    
        return $form;
      }

    In my opinion uncommented code is bad. It tends to stick around for ever. At least add a @todo or just remove it.

  • README.txt
    Has not been updated to D8.
    Add a @todo and/or do a quick update.
franskuipers’s picture

Thanks for your patch and review.
I am quite busy these holidays, but will apply this ASAP.

ayalon’s picture

Is there somewhere a sandobox with the latest version? Can someone create a new dev release so we can work on the port more easily?

pwolanin’s picture

Patch doesn't apply to latest 7.x-1.x

error: patch failed: clone.module:1
error: clone.module: patch does not apply
error: patch failed: clone.pages.inc:1
error: clone.pages.inc: patch does not apply
error: patch failed: clone.rules.inc:1
error: clone.rules.inc: patch does not apply
piper:node_clone pwolanin$

pwolanin’s picture

I made a 7.x-1.0 release (finally), so now is a good time to start. the existing patch here might have some useful bits, but is rather old at this point.

I just renamed the file and function, and ran drupalmoduleupgrader over it.

Feel free to take the 8.x-1.x branch and start trying to make it work.

Anonymous’s picture

I have an alternate module I made as part of a work project that does the same thing as Node Clone if someone needs an interim D8 solution. https://www.drupal.org/node/2636002

DamienMcKenna’s picture

Title: Upgrade node clone module to D8 » Port Node Clone to Drupal 8

Standardized the issue title.