Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#18 | 2959336-18-store-path.patch | 18.29 KB | bojanz |
#15 | interdiff-2959336-9-15.txt | 3.44 KB | mglaman |
#15 | 2959336-15.patch | 4.1 KB | mglaman |
| |||
#9 | pathauto_integration-2959336-9.patch | 4.07 KB | joekers |
| |||
#5 | pathauto_integration-2959336-5.patch | 3.67 KB | kala4ek |
Comments
Comment #2
kala4ekComment #4
joekersTested 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:
Should this be 'path' like in the other BaseFieldDefinition in this patch?
The description might be better as "The store path."? Same goes for the other BaseFieldDefinition description.
Comment #5
kala4ekThanks for review @joekersm.
Here is updated patch
Comment #7
joekersLooks 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.
Comment #8
mglamanNeed 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.
Comment #9
joekersI'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.Comment #10
mglamanWe'll need to add it to CommerceKernelTestBase, since it's now a dependency of the store module (which installs store.)
Comment #11
joekersSorry I'm not sure what you mean, could you explain? I've added 'path' inside the
$modules
array inCommerceKernelTestBase
- the tests seem to be passing now.Comment #12
mglamanSorry, I meant you were right in adding it to the base class there :)
Comment #13
joekersAh ok cool, thanks for reviewing :)
Comment #14
mglamanAll other fields use "URL alias" and "The X URL alias"
Stores are not revisionable
Need short array syntax
Comment #15
mglamanRerolled.
Comment #16
bojanz CreditAttribution: bojanz at Centarro commentedThe feature makes sense, the patch makes sense. Let's proceed.
Only one question:
Why are we adding a path entity key? Neither core nor other Commerce entities do that.
Comment #17
bojanz CreditAttribution: bojanz at Centarro commentedThe entity key needs to be removed.
The update hook is misnumbered, should be 8201, not 8001.
There is no install() method on the module handler, we need to use the module installer.
This weight puts the url alias in the middle of the form. Let's put it after the tax settings, at the very bottom.
Comment #18
bojanz CreditAttribution: bojanz at Centarro commentedNew patch. Also cleans up tests, which no longer need to enable "path" themselves. Plus, we add the same help text we have on products.
Comment #19
bojanz CreditAttribution: bojanz at Centarro commentedBetter title.
Comment #22
bojanz CreditAttribution: bojanz at Centarro commentedAll tests passed. Tested manually with and without pathauto as well.
Comment #23
tormi