If a D7 port ever happens, the module's main module files (probably user_relationships_api) need to be moved to the main directory so that drupal_get_path() will identify the module correctly. A bug has been filed with other modules (Rules, Entity API) that have the same problem to get them fixed too.

With D6 and older having the main module in a subdirectory wasn't a problem, there was no reason in Drupal core to be able to clearly identify this. Drupal 7, on the other hand, brings with it the Update Manager that relies upon drupal_get_path() to tell where a module's main directory is and leads to broken installations when the directory module's structure is needlessly convoluted.

CommentFileSizeAuthor
#4 rename_api_module.patch132.67 KBberdir

Comments

alex.k’s picture

Added a link to this issue to #671816: Drupal 7 port which is the main bet of a D7 port.

berdir’s picture

What exactly is the "main module"? :)

UR doesn't have a module that matches the project name, can the update manager actually resolve this?

alex.k’s picture

The "main" module is user_relationships_api. I think that the directory structure, and also the fact that many functions do not start with user_relationships_api_* come from the original 5.x version, and these issues were never corrected in the 6.x branch.

I am not sure we can rename the module to match the project name. We can, however, add a dummy module if that makes migration easier.

berdir’s picture

Status: Active » Needs review
StatusFileSize
new132.67 KB

Ok, here is a patch.

- Moves user_relationships_api into the top-level folder
- Rename it to user_relationships
- Added an update function to user_relationships_ui, that automatically enabled user_relationships and also makes sure that it doesn't try to install the schema again
- Move the content of the api.inc into .module. There is no point in having separate files when you need to include them on every single page anyway.

I haven't moved over the admin UI yet, that should be possible, but this page is already huge. Will create a follow-up issue for that.

When you test this patch, make sure to run update.php before accessing the site directly. It *will* blow up if you don't.

mrf’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Needs review » Reviewed & tested by the community

Git apply gave me a couple of minor errors.

rename_api_module.patch:754: trailing whitespace.
 * 
rename_api_module.patch:565: new blank line at EOF.
+
warning: 2 lines add whitespace errors.

Looked over the patch and seems like there's not much happening but files moving and dependencies changing.

Applied the patch and spent a while clicking through the site, created some new relationship types, added some new relationships, and worked with some existing ones and I didn't come across any issues. I'd say this is ready to go.

I did get a warning from my user_relationship_limits module about the missing ur_api dependency, that will be up to module maintainers to fix but is worth noting.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, rename_api_module.patch, failed testing.

berdir’s picture

Status: Needs work » Reviewed & tested by the community

Thanks for testing and reviewing.

Yes, I'm just moving stuff around, so that's why git apply shows some warnings.

BTW, the testbot is now enabled but obviously heavily confused by this patch. We'll see how well it goes after I've commited this. This will help prevent obvious bugs like the one that resulted in a PDOException when no relationship types were defined. It also encourages to write more tests :)

BenK’s picture

Subscribing

BenK’s picture

I tested the patch in #4 and ran update.php. Everything worked smoothly, no problems at all. So I think this ready to be committed.

--Ben

P.S. I know that you're not a big fan of the "UR" prefix in sub-module names. I don't care for that either. Do you want to change sub-module names to use "User Relationships" in a subsequent issue? We could also move over the admin UI in that same issue.

berdir’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -drupal 7

Thanks for testing, commited.

@BenK: Changing the visible module name has no side-effects, it is only used in the user interface. So we can change that any time we want, no reason to delay an alpha version because of that.

Status: Fixed » Closed (fixed)

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