Right now, store entities can have own pages, but the paths cannot be configured.
I think that Store module should be integrated with Path.
It will also allow configuring patterns with Pathauto module.

Mark this issue as a major, because it has a large impact on UX.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kala4ek created an issue. See original summary.

kala4ek’s picture

Assigned: kala4ek » Unassigned
Status: Active » Needs review
Issue tags: -undefined
FileSize
3.66 KB

Status: Needs review » Needs work

The last submitted patch, 2: pathauto_integration-2959336-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

joekers’s picture

Tested the patch manually and it allowed me to set a store URL alias.

Also with the Pathauto module enabled it allowed me to set up a pattern to use on all store entities.

So everything worked as expected but I do have some questions:

+++ b/modules/store/commerce_store.install
@@ -0,0 +1,31 @@
+  $storage_definition = BaseFieldDefinition::create('string')

Should this be 'path' like in the other BaseFieldDefinition in this patch?

+++ b/modules/store/commerce_store.install
@@ -0,0 +1,31 @@
+    ->setDescription(t('The media path.'))

The description might be better as "The store path."? Same goes for the other BaseFieldDefinition description.

kala4ek’s picture

Thanks for review @joekersm.
Here is updated patch

Status: Needs review » Needs work

The last submitted patch, 5: pathauto_integration-2959336-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

joekers’s picture

Looks good! One thing left is the failing tests - seems to be the same thing on every test failure so once it's fixed for one it should fix them all:

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "path" plugin does not exist.

Could this be due to the integration with the Pathauto module? I should have some time later to look into this further.

mglaman’s picture

Priority: Major » Normal

Need to add path module to tests as a dependency. Also, it's a feature and I'd mark this as "normal." It's becoming more popular as people build store locators.

joekers’s picture

Status: Needs work » Needs review
FileSize
4.07 KB

I've added the path module to CommerceKernelTestBase but I'm not sure if this is the best place, if not I can add to each individual test file that needs it.

mglaman’s picture

We'll need to add it to CommerceKernelTestBase, since it's now a dependency of the store module (which installs store.)

joekers’s picture

Sorry I'm not sure what you mean, could you explain? I've added 'path' inside the $modules array in CommerceKernelTestBase - the tests seem to be passing now.

mglaman’s picture

Sorry, I meant you were right in adding it to the base class there :)

joekers’s picture

Ah ok cool, thanks for reviewing :)

mglaman’s picture

Status: Needs review » Needs work
  1. +++ b/modules/store/src/Entity/Store.php
    @@ -236,6 +237,17 @@ class Store extends ContentEntityBase implements StoreInterface {
    +      ->setLabel(t('Path'))
    +      ->setDescription(t('The store path.'))
    

    All other fields use "URL alias" and "The X URL alias"

  2. +++ b/modules/store/src/Entity/Store.php
    @@ -236,6 +237,17 @@ class Store extends ContentEntityBase implements StoreInterface {
    +      ->setRevisionable(TRUE)
    

    Stores are not revisionable

  3. +++ b/modules/store/src/Form/StoreForm.php
    @@ -15,6 +15,21 @@ class StoreForm extends ContentEntityForm {
    +        'class' => array('path-form'),
    ...
    +      '#attached' => array(
    +        'library' => array('path/drupal.path'),
    

    Need short array syntax

mglaman’s picture

Status: Needs work » Needs review
FileSize
4.1 KB
3.44 KB

Rerolled.

bojanz’s picture

The feature makes sense, the patch makes sense. Let's proceed.

Only one question:

+ *     "path" = "path"

Why are we adding a path entity key? Neither core nor other Commerce entities do that.

bojanz’s picture

Status: Needs review » Needs work

The entity key needs to be removed.

The update hook is misnumbered, should be 8201, not 8001.

+  $module_handler = \Drupal::service('module_handler');
+  if (!$module_handler->moduleExists('path')) {
+    $module_handler->install(['path']);
+  }

There is no install() method on the module handler, we need to use the module installer.

+    $form['path_settings'] = [
+      '#type' => 'details',
+      '#title' => $this->t('URL path settings'),
+      '#open' => !empty($form['path']['widget'][0]['alias']['#value']),
+      '#group' => 'advanced',
+      '#attributes' => [
+        'class' => ['path-form'],
+      ],
+      '#attached' => [
+        'library' => ['path/drupal.path'],
+      ],
+      '#weight' => 30,
+    ];
+    $form['path']['#group'] = 'path_settings';

This weight puts the url alias in the middle of the form. Let's put it after the tax settings, at the very bottom.

bojanz’s picture

Status: Needs work » Needs review
FileSize
18.29 KB

New patch. Also cleans up tests, which no longer need to enable "path" themselves. Plus, we add the same help text we have on products.

bojanz’s picture

Title: Add a "path" field for pathauto compatibility » Add a "path" field to stores

Better title.

Status: Needs review » Needs work

The last submitted patch, 18: 2959336-18-store-path.patch, failed testing. View results

  • bojanz committed 8b797fe on 8.x-2.x authored by kala4ek
    Issue #2959336 by kala4ek, mglaman, joekers, bojanz: Add a "path" field...
bojanz’s picture

Status: Needs work » Fixed

All tests passed. Tested manually with and without pathauto as well.

tormi’s picture

Status: Fixed » Closed (fixed)

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