Problem/Motivation

Follow-up to #813754: Ability to set variant machine name in Panels UI

Ability to set variant machine name in Panels UI for Cloning and Importing variants.

Proposed resolution

Explicitly set a machine-name when importing and cloning variants.

Behaviour Variant Clone

  • When using an existing machine_name, you get a validation error before saving. Message is something like "The machine-readable name is already in use. It must be unique."

Behaviour Variant Import

  • When using an existing machine_name + option 'Overwrite=FALSE', you get a validation error before saving. Message is something like "The machine-readable name is already in use. It must be unique."
  • When using an existing machine_name + option 'Overwrite=TRUE', the existing variant will be deleted before saving the new variant. No errors.
  • When using an non-existing machine_name + option 'Overwrite=FALSE', the new variant will be saved.
  • When using an non-existing machine_name + option 'Overwrite=TRUE', the new variant will be saved.

Additional behaviour Variant Import regarding UUID's

  • Re-generate UUIDs if machine-name is different than import-code.
  • Keep UUIDs of import-code if machine-name is equal than import-code.

Remaining tasks

User interface changes

  • Machine-name field and overwrite checkbox added to panel-variant import-form.
  • Machine-name field added to panel-variant clone-form.

API changes

  • none
CommentFileSizeAuthor
#20 ability_to_set_variant-2477637-20.patch9.83 KBmichelle
#19 ability_to_set_variant-2477637-19.patch9.82 KBjoelpittet
#17 interdiff-2477637-15-17.txt1.98 KBndf
#17 ctools-Ability to set variant machine name in Panels UI for Cloning variants-2477637-17.patch9.81 KBndf
#15 interdiff-2477637-09-15.txt3.37 KBndf
#15 ctools-Ability to set variant machine name in Panels UI for Cloning variants-2477637-15.patch8.13 KBndf
#9 ctools-Ability to set variant machine name in Panels UI for Cloning variants-2477637-09.patch5.05 KBndf
#9 interdiff-2477637-08-09.txt953 bytesndf
#8 ctools-Ability to set variant machine name in Panels UI for Cloning variants-2477637-08.patch4.67 KBndf
#8 interdiff-2477637-04-08.txt1.65 KBndf
#4 ctools-Ability to set variant machine name in Panels UI for Cloning variants-2477637-4.patch3.61 KBndf
#2 ctools-Ability to set variant machine name in Panels UI for Cloning variants-2477637-2.patch1.89 KBndf
User_Orders_Page.png55.71 KBjoelpittet

Comments

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: +needs forward port to Drupal 8
ndf’s picture

ndf commented
Status: Active » Needs review
StatusFileSize
new ctools-Ability to set variant machine name in Panels UI for Cloning variants-2477637-2.patch1.89 KB

Branch on github: https://github.com/ndf4/ctools/tree/issue/2477637-MachineNameWhenCloning...

Note that I had to setup $form_state['task_name'] explicitly, because the default had an - instead of an underscore in task_name.

ndf’s picture

ndf commented
Status: Needs review » Needs work

Also needed when "importing variants".

ndf’s picture

ndf commented
Status: Needs work » Needs review
StatusFileSize
new ctools-Ability to set variant machine name in Panels UI for Cloning variants-2477637-4.patch3.61 KB

Added functionality for machine-name in both Cloning and Importing handlers/variants.

ndf’s picture

ndf commented
Title: Ability to set variant machine name in Panels UI for Cloning variants. » Ability to set variant machine name in Panels UI for Cloning and Importing variants.
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs review » Reviewed & tested by the community

Thanks @ndf gave it a whirl and it works like a charm!

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Reviewed & tested by the community » Needs work

Whoops, try this:

Clone a variant, change the machine name to anything else and the title to the same as another variant.
The new machine name is not saved and we get an integrity constraint error page:

Integrity constraint violation: 1062 Duplicate entry 'pm_existing_pages_user_login__test' for key 'name': INSERT INTO {page_manager_handlers} (name, task, subtask, handler, weight, conf) VALUES

ndf’s picture

ndf commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2477637-04-08.txt1.65 KB
new ctools-Ability to set variant machine name in Panels UI for Cloning variants-2477637-08.patch4.67 KB

Hi Joel,

This is an updates patch that should resolve your issues.

The patch acts on:
- Variant Clone
- Variant Import

Behaviour Variant Clone:
- When using an existing machine_name, you get a validation error before saving. Message is something like "The machine-readable name is already in use. It must be unique."

Behaviour Variant Import:
- When using an existing machine_name + option 'Overwrite=FALSE', you get a validation error before saving. Message is something like "The machine-readable name is already in use. It must be unique."
- When using an existing machine_name + option 'Overwrite=TRUE', the existing variant will be deleted before saving the new variant. No errors.
- When using an non-existing machine_name + option 'Overwrite=FALSE', the new variant will be saved.
- When using an non-existing machine_name + option 'Overwrite=TRUE', the new variant will be saved.

ndf’s picture

ndf commented
StatusFileSize
new interdiff-2477637-08-09.txt953 bytes
new ctools-Ability to set variant machine name in Panels UI for Cloning variants-2477637-09.patch5.05 KB

Also added solution for "The UUIDs of imported variant and relative panes are the same of original ones".
I added it here, because this functionality (create unique uuids) depends on having a machine-name.

ndf’s picture

ndf commented
Issue summary: View changes
ndf’s picture

ndf commented
Related issues: +#2449245: Cloning page manager handlers leads to duplicated uuids.
ndf’s picture

ndf commented
Related issues: +#2253919: The UUIDs of cloned variant and relative panes are the same of original ones
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Still getting this nice doozie:

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'page_user_wishlist__panel' for key 'name': INSERT INTO {page_manager_handlers} (name, task, subtask, handler, weight, conf) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5); Array ( [:db_insert_placeholder_0] => page_user_wishlist__panel [:db_insert_placeholder_1] => page [:db_insert_placeholder_2] => user_wishlist [:db_insert_placeholder_3] => panel_context [:db_insert_placeholder_4] => 2 [:db_insert_placeholder_5] => a:11:{s:5:"title";s:5:"Panel";s:9:"no_blocks";i:0;s:8:"pipeline";s:8:"standard";s:22:"body_classes_to_remove";s:0:"";s:19:"body_classes_to_add";s:0:"";s:6:"css_id";s:0:"";s:3:"css";s:0:"";s:8:"contexts";a:0:{}s:13:"relationships";a:0:{}s:4:"name";s:5:"panel";s:3:"did";s:2:"98";} ) in drupal_write_record() (line 7316 of html/includes/common.inc).

Maybe need some validation?

All I'm doing is cloning the variant and giving the title the same as the previous, and boom!

Thanks for covering off the UUID bit that should be nice for imports:)

ndf’s picture

ndf commented
Status: Needs review » Needs work

Thinks this needs tests and updates for panes without readable machine-name based on title.

This patch kinda depends on a panels-patch https://www.drupal.org/node/2253919#comment-9702919

And this part should take care the "save" operation

  // Delete existing handler if 'overwrite' option is checked.
  if ($form_state['values']['overwrite'] == TRUE && $existing_handler = page_manager_load_task_handler($handler->task, NULL, $handler->name)) {
    page_manager_delete_task_handler($existing_handler);
  }

  // Setup handler configuration.
  $handler->conf['name'] = $form_state['values']['name'];

And without deleting it breaks.

ndf’s picture

ndf commented
Status: Needs work » Needs review
StatusFileSize
new ctools-Ability to set variant machine name in Panels UI for Cloning variants-2477637-15.patch8.13 KB
new interdiff-2477637-09-15.txt3.37 KB

Hi Joel, validation was definitely necessary.

Multiple things have changed:

  1. page_manager_handler_add_to_page now does not regenerate the $handler->name from the title. It is required now, and only _import_submit() and _clone_submit() are calling it. The main reason is that regeneration regex was different than formapi's machinename one. And I think it is redundant now.
  2. Validation and submit code is now equal for both _import and _clone. It give a nice form-error if the machine name exists (and if you did not tick 'overwrite' when importing.

I have tested importing and cloning with new and existing machine-names and it looks quite good to me. The interface/UI is clear and consistent, the exported variants (in features) looks even better.

Note that the 're-generate' UUID's depends on patches:
- ctools: https://www.drupal.org/node/2449245
- panels: https://www.drupal.org/node/2253919
But the machine-name part does work without it.

ndf’s picture

ndf commented
Status: Needs review » Needs work

One task to go, if you create a new variant Add variant, the machine-name set must be used.
Currently a uuid is used.

ndf’s picture

ndf commented
Status: Needs work » Needs review
StatusFileSize
new ctools-Ability to set variant machine name in Panels UI for Cloning variants-2477637-17.patch9.81 KB
new interdiff-2477637-15-17.txt1.98 KB

Now also machine-names including validation for "Add new variant" operation.

ndf’s picture

ndf commented
Status: Needs review » Needs work
Related issues: +#2445203: Error trying to clone a variant

https://www.drupal.org/node/2445203#comment-9948477 is just committed and touches same code for generating machine-name for a handler.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Status: Needs work » Needs review
StatusFileSize
new ability_to_set_variant-2477637-19.patch9.82 KB 
  1. +++ b/page_manager/page_manager.module
@@ -472,10 +472,6 @@ function page_manager_handler_add_to_page(&$page, &$handler, $title = NULL) {
-    $handler->conf['name'] = preg_replace('/[^a-z0-9_]+/', '-', strtolower($title));

    Trim was wrapped around this in the security release. 

  2. +++ b/page_manager/page_manager.admin.inc
@@ -1304,6 +1304,27 @@ function page_manager_handler_add($form, &$form_state) {
+
+
+  // Validate if new machine-name exists.

    Removed extra line break.

michelle’s picture

michelle
she/her
Primary language English
Location Wisconsin, USA
commented
StatusFileSize
new ability_to_set_variant-2477637-20.patch9.83 KB

The patch applies but with a little "fuzz" so I re-diff'd it against the current 7.x-1.x code. I didn't make any other changes.

Before applying this patch, I cloned a variant and gave it the same name as the original. When I exported them to look at the code, the machine name of the clone was blank. After applying the patch, I can set the machine name both when cloning and importing to avoid that problem. Seems to be working good but leaving at "needs review" since I technically changed the patch, even if it's just updating it.

rivimey’s picture

rivimey
she/her
commented
Status: Needs review » Needs work

Thanks Michelle!

A quick review:

DCS:
- there are multiple cases of very long lines which should be wrapped or modified.
- the docblock comments have insufficient and occasionally confusing information, notably no @param or @return args.

Other
- the 'does-name-exist' code should be refactored into its own function. Before adding own function please check there isn't a suitable one elsewhere (see #18)

  1. +++ b/page_manager/page_manager.admin.inc
@@ -1304,6 +1304,26 @@ function page_manager_handler_add($form, &$form_state) {
+ * Validate clone an existing task handler into a new handler.

    This docblock needs additional information. Are we validating a clone of something, validating that it might be possible to clone something, or something else.

    Also needs @param, @return elements, and preferably an extended description. 

  2. +++ b/page_manager/page_manager.admin.inc
@@ -1304,6 +1304,26 @@ function page_manager_handler_add($form, &$form_state) {
+    form_error($form['shoveoff'], t('You account permissions do not permit you to import.'));

    Not keen on this key name: 'shoveoff'... wouldn't 'message' or similar be adequate? 

  3. +++ b/page_manager/page_manager.admin.inc
@@ -1304,6 +1304,26 @@ function page_manager_handler_add($form, &$form_state) {
+  $new_handler_name = $task_id . "_" . $subtask_id . "__" . $form_state['values']['name'];

    Line too long.

    What happens if $form_state['values'] is empty or unset? It would seem sensible to include:
    if ('name' not empty) {
    rest of function
    } 

  4. +++ b/page_manager/page_manager.admin.inc
@@ -1406,10 +1426,12 @@ function page_manager_handler_add_form($form, $form_state, $features = array())
+      '#description' => t('Enter the name of the new variant.'),

    I think we need to keep 'administrative' in the description.

    Have we actually lost the 'auto assign' functionality now? If not, please don't remove that from the description either. If so, please ensure it is called-out in the upgrade documentation somewhere. 

  5. +++ b/page_manager/page_manager.admin.inc
@@ -1466,21 +1488,51 @@ function page_manager_handler_add_form($form, $form_state, $features = array())
+  if (isset($form_state['values']['overwrite']) && $form_state['values']['overwrite'] == TRUE) {

    DCS: Suggest break line before "&&" and just before ") {" a the end of line. 

  6. +++ b/page_manager/page_manager.admin.inc
@@ -1466,21 +1488,51 @@ function page_manager_handler_add_form($form, $form_state, $features = array())
+ * Import a handler from code and save it into the panel.

    DCS:
    - Import "a" hander : any old handler? please be more specific.

    - Needs @param, @return parts, extended description. 

  7. +++ b/page_manager/page_manager.admin.inc
@@ -1521,15 +1575,44 @@ function page_manager_handler_import_validate($form, &$form_state) {
+  $new_handler_name = $handler->task . "_" . $handler->subtask . "__" . $form_state['values']['name'];

    This code is (near) duplicate of code earlier (L1317..L1324) and later. Please refactor into a sub-function. 

  8. +++ b/page_manager/page_manager.admin.inc
@@ -1521,15 +1575,44 @@ function page_manager_handler_import_validate($form, &$form_state) {
+ * Import a handler from code and save it into the panel.

    As earlier... 

  9. +++ b/page_manager/page_manager.admin.inc
@@ -1521,15 +1575,44 @@ function page_manager_handler_import_validate($form, &$form_state) {
+  // Re-generate UUIDs if machine-name is different that import-code.

    Language:
    'if machine-name is different that import-code'

    --> 'if the machine name is not "import-code".'

    or perhaps:

    --> 'if the machine name has changed since the code was imported.' ? 

  10. +++ b/page_manager/page_manager.admin.inc
@@ -1521,15 +1575,44 @@ function page_manager_handler_import_validate($form, &$form_state) {
+  if ($form_state['values']['overwrite'] == TRUE && $existing_handler = page_manager_load_task_handler($handler->task, NULL, $handler->name)) {

    DCS: Line too long. Suggest two nested if's. 

  11. +++ b/page_manager/page_manager.admin.inc
@@ -1664,28 +1747,85 @@ function page_manager_handler_export($form, &$form_state) {
+ * Clone an existing handler and save it into the panel.

    @param, @return, etc. 

  12. +++ b/page_manager/page_manager.admin.inc
@@ -1664,28 +1747,85 @@ function page_manager_handler_export($form, &$form_state) {
+ * Validate clone an existing task handler into a new handler.

    See earlier comments about 'validate'... what is this doing, and why have we got two functions with the same Summary-description and different function names? 

  13. +++ b/page_manager/page_manager.admin.inc
@@ -1664,28 +1747,85 @@ function page_manager_handler_export($form, &$form_state) {
+    form_error($form['object'], t('Unable to get a variant from the import. Errors reported: @errors', array('@errors' => $errors)));

    Line too long 

  14. +++ b/page_manager/page_manager.admin.inc
@@ -1664,28 +1747,85 @@ function page_manager_handler_export($form, &$form_state) {
+  // Validate if new machine-name exists.

    Use newly refactored 'name exists' code once more... 

  15. +++ b/page_manager/page_manager.module
@@ -477,10 +477,6 @@ function page_manager_handler_add_to_page(&$page, &$handler, $title = NULL) {
-    $handler->conf['name'] = trim(preg_replace('/[^a-z0-9_]+/', '-', strtolower($title)), '-');

    Sorry if this is a stupid Q but why is this code no longer needed?

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Parent issue: #813754: Ability to set variant machine name in Panels UI » #3195061: Plan for CTools 7.x-1.19 release
Related issues: +#813754: Ability to set variant machine name in Panels UI
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Parent issue: #3195061: Plan for CTools 7.x-1.19 release » #3195523: Plan for CTools 7.x-1.21 release
joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Parent issue: #3195523: Plan for CTools 7.x-1.21 release » #3338139: Plan for CTools 7.x-1.22 release