Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Related Issues
Issue blocker: #1551346: [META] Port sandbox vs. full project functionality
Problem/Motivation
We need to either port the D6 stuff that magically creates node aliases for projects using the machine_name, or migrate to pathauto or something. This issue is for deciding what to do and doing it. ;)
Comments
Comment #1
iamcarrico commentedI went through and made a slight code change to use both ideas. If pathauto is not installed, then the module will automatically assign a path alias when a project node is created. Once the pathauto module is enabled however, the alias will be added to the logic of the pathauto module automatically, and the project module will stop creating aliases. Fairly straight-forward, and will increase usability in the future.
Comment #3
iamcarrico commentedTest failed due to the path module not being enabled, and thus the $node->path did not exist in the object. This should be corrected by the path module being enabled.
Comment #5
iamcarrico commented#3: d7-solution-for-project-shortname-aliases-1551344-3.patch queued for re-testing.
Comment #7
iamcarrico commentedMade the changes that are failing in the patch--- currently unable to test on my own machine so will be using this to run tests as well.
Comment #8
dwwCool, thanks for jumping in and also for getting the tests passing!
However, as I wrote in the original post, the first step is figuring out what we want to do. ;) I'm not convinced any of this effort is worth doing. Maybe the solution is to completely ignore path aliases inside project.module and let people set them manually or via pathauto, just like for every other node type on their site. One of the goals of the D7 port is to remove as much project.module-specific magic as possible and let project nodes just be like any other content in a normal Drupal site.
But, if we are going to keep doing magic here, my concerns with the patch in #7:
A) This patch is assuming a single node type called "project". That matches the default type provided by project.module when you install it, but it's not going to handle any other node types that have a "Project type" field and therefore behave like projects. Seems a bit weird to handle this automatically in one case, but punt to the site builder for any other types. Almost seems better to just ignore path aliases entirely if pathauto is installed and let site builders set this up how they want for all node types using the standard workflow, instead of special-casing this one node type.
B) It seems a bit wonky to set the pathauto variable both when project.module is installed, and when pathauto is enabled. I guess that's to cover the case that project.module is installed first, then pathauto is enabled, and I suppose there's no harm in setting it twice here since you're not going to be clobbering custom configuration. But, if we're going to keep it, it'd be nice to have a code comment that more explicitly describes why we're doing this twice.
C) If we were to continue with this approach, will pathauto delete the 'pathauto_node_project_pattern' variable on uninstall or would that be our responsibility to cleanup after ourselves?
D) There are some code style bugs in here:
ifinsideproject_node_insert()isn't properly indented.ifand the conditional inif(in_array('pathauto', $modules)) {.return ;.} else {should be:'whenever possible. No reason to mix and match with"all over this patch. The only reasons to use"are if you're expanding a variable directly inside the string, or you want to see'inside the string and don't want to have to escape it.* Sets pathauto if the module has been enabled.-- huh? ;)* Sets the path of the node if pathauto is not.-- huh? ;)* Implements hook_modules_installed.-- should match the others and look likeImplements hook_modules_installed().E) Instead of forcing path.module to be a requirement for project.module, it'd be better if all this alias stuff was just conditional on if path.module is enabled. We're already sort of doing that inside
project_create_node_alias(). However, it seems weird to test for pathauto outside of that function at each call site, but to test for path.module inside the function. I think it'd be cleaner to just have that function check for both pathauto and path, only do anything if path is enabled and pathauto is not, and remove the checks at the call sites.Normally I wouldn't have given a thorough review on a patch that we might never commit at all, but since you're new here in the project issue queue and will hopefully be contributing a lot more over the next weeks and months, I wanted to use this as a training opportunity. I hope it was useful!
Let me know what you think and if you have any questions...
Thanks!
-Derek
Comment #9
iamcarrico commentedYeah, it was just easier to code out what I thought we should do rather than explain it. It is not a very hard code, so somewhat made sense to just go forward and see where it led me.
My thoughts:
1) The more and more I look into this patch, the more I think that is not entirely necessary. I started work on this partially because I knew it was simple/could see how the project module was being built from the inside while doing it... but there are too many odd exceptions. Kind of to your point in (A), but since this module assumes that if you are making a project node (without pathauto)--- then you want the pre-fix to be "project/". Not a bad idea necissarily, especially since I would bet most who use this will also be using pathauto, but it is somewhat unnecessary.
B) As to your point about setting the pathauto variable twice--- it checks during the install to see if the pathauto module is indeed enabled. If it is not, no actions are taken, thus making it necessary to run it again if pathauto is later installed. I see an issue with this however if the content_type for project changes in the future and is not just project. There can be no harm with leaving the install hook however, as we are creating that content type on our own.
III) Forcing the path module to be a requirement of the module was a bit of code I put in to try and get the simpletests to work, I removed it. As to the checking within the function, I decided to do it that way in case this function gets called at any other point or within future development. As the code in the function will break if the path module is disabled, it seemed a better idea to do the check there to prevent future errors.
iv) On a different note--- we are creating a node type to be projects and issues, and setting them up with certain fields that I assume will be used throughout the module as a base for features. My question is: what if the user deletes a field? For example, from what I can tell if the user deletes "project type" then the node type will no longer be considered a "project". This seems kind of odd, as maybe some future implementations of this will not need a distinction between project and sandbox--- or perhaps would rather express it in a different manner. It might be a better idea to either go the route OG did, and create on the content type page a way to make any node type into a project or issue. OR create a entity bundle that is just a "project", more like Taxonomy. I know this isnt the best threat to discuss it... but a thought before we travel too far down the rabbit hole.
5) I fixed the grammar issues, still need to master the art of Drupal code-standardification.
-Ian
Comment #10
iamcarrico commentedComment #11
dave reidI'm almost thinking it would be better to remove the alias functionality and ship a default pathauto pattern variable with the featurized versions which include the project node type.
Comment #12
dwwBump. #1551346: [META] Port sandbox vs. full project functionality is nearing completion, and one of the only remaining bugs is that if you configure the site to auto-generate shortnames for sandboxes, the shortname field is still visible in the UI. Basically, that setting is meaningless at this point. Not sure if that's worthy of a separate bug or if we should just hash it out as part of this. It'd be great to fully wrap up all this sandbox stuff ASAP. Thoughts?
Thanks,
-Derek
Comment #13
senpai commentedI'm gonna say that @sdboyer gets to make the final call on this piece of the user experience. But yeah, it shouldn't be a separate bug; at this point we can just handle it in here.
Comment #14
cweagansI think it makes a lot of sense to just use pathauto.
Comment #15
senpai commentedBumping to critical because we need this for the Drupal.org D7 Upgrade re-launch.
Comment #16
dwwBased on a quick chat, drumm says:
Comment #17
drummLooking over this again, I'm having a hard time coming up with a downside to pathauto. I thought maybe this would be creating a lot of new aliases, and something else to performance test; but we have aliases anyway.
I think we can go with something cleaning up the latest patch here, just setting the pattern for all project node types when installed with pathauto. Sandbox projects can use hook_pathauto_alias_alter() for a separate sandbox project alias.
Comment #18
drummI committed a first pass at this: http://drupalcode.org/project/project.git/commitdiff/e1dce546fc8c8a559b1...
It creates a helper function, _project_set_pathauto_defaults(), which sets a default alias for all project types, not clobbering any existing setting. This is called when
Leaving open for sandboxes.
Comment #19
drummMade another commit on this, http://drupalcode.org/project/project.git/commitdiff/5d5b0433762ca9d2bd6..., which is believe closes this out. Project has the functionality for a separate sandbox project alias, which it does not implement. Drupalorg customizations does set pathauto variables to use it.
Comment #20.0
(not verified) commentedIssue blocker: #1551346: [META] Port sandbox vs. full project functionality