System table not updated when moving modules

I encountered this problem (using 4.7.0b4) when moving modules from Drupal's main modules folder to a modules folder under the sites directory. It also happened to be the taxonomy_access module in my case, and deleting the row from system solved the problem.

The function that retrieves system file locations from the database (system_get_files_database) wasn't checking to see if the file's path had changed. With the patch, if module files have moved their new location will be detected when the site admin visits the modules page.

Can you try a unified patch file, diff -u original.file new.file > mypatch.patch

k

Heh ;o] a better patch file. Sorry for the floundering I'm a bit new to submitting patches.

Tested on a 4.7.0b4 site, and it worked fine. For completeness, you should also set $file->filename when you're changing it in the database so that the current request uses the right filename. The attached patch adds that.

chrisd: definitely file the bugs. As a software engineer, you know the value of giving clear instructions to reproduce bugs, and that makes fixing them so much easier. Patches can be tested by others as time permits.

Related to http://drupal.org/node/46383 ?

Yes, this appears to be the same problem, with different solutions. I think a combination of both would be good.

It might be worth adding a watchdog command to this patch, saying that the system table has been updated. I can do that if anyone else thinks it's worth it.

Yes, it appears we had the same scenario. I will close out my issue in favor of this one.

@smsimms: Are we sure that $files[$file->name]->filename exists before updating the database? If so, can we add a watchdog comment that the file location was updated?

Thanks,
samo

Yes, the function gets passed a list of files based on a directory listing (search the system.module file for calls to the system_get_files_database function to see how the lists are made).

I'm running out for a while now, but I can add the watchdog command to the patch later on this evening if someone else doesn't beat me to it (feel free).

Please mark one of the issues as duplicate, and continue with one issue instead. Let's go with the solution that has the smallest performance penalty, or that is otherwise considered to be cleanest. I want you guys to figure out the best approach. :)

+1

Patch works great with beta 4, and fixes the problem described. We'd experienced this one moving sites to our new server recently: we've changed from 'normal', separate Drupal sites where contrib modules were stored under modules/, to a multi-site setup where each sites' modules now reside in sites/default/modules/. At one point we even managed to get our servers IP in the path... :o\

Anyway, thats a bit OT. Patch works great. Not sure of the performance hit of the additional query, but as long as this is called only on admin/modules, there shouldn't be much of a problem. Ready to apply, methinks.

Here's one with the watchdog call added. I'd say this is now a NOTICE, rather than an ERROR. Also, I couldn't think of a nicer message... please review.

Dang it.

Two things:

1. The message gives the new location, not the old location. This could be confusing to users who move a module and then see a message ostensibly saying that the module has moved, again.

2. The admin has to open and save the modules page for this to work. Should it be invoked whenever a module cannot be found? Say on a bad page view.

Otherwise, it's a good fix.

Two things:

1. The message gives the new location, not the old location. This could be confusing to users who move a module and then see a message ostensibly saying that the module has moved, again.

2. The admin has to open and save the modules page for this to work. Should it be invoked whenever a module cannot be found? Say on a bad page view.

Otherwise, it's a good fix.

I was literally changing the patch to give both filenames when you wrote this. :-)

Also, the first query in the patch needs to be removed. I'll have a new patch uploaded in a few minutes.

Steve

Here's the updated patch, against current CVS.

@mfredrickson: With regard to your second comment, the modules page doesn't have to be saved, just opened (or any other activity that triggers a list of modules to be built from the filesystem). Having the list rebuilt every time there's a 404 error would probably get resource-heavy, which may be why it isn't being done now. By all means, file an issue if you think that should be done differently, but I don't think it needs to be a part of this fix.

Steve

Looks good to me: tested, working fine. Not quite convinced about the message itself - perhaps we need to explain the message a little, eg: "%oldfile was moved to %newfile. System settings changed accordingly". If no one else agrees, I'd say this was RTC.

Attached patch is a very, very minor tweak of smsimms', to include 2 spaces for indentation on a couple of lines, as per coding standards.

We do a file_exists() check for every module include anyway. Why not move the code there? It does not add any additional performance penalty if no modules are missing.

-1 on this patch
We should be using drupal_get_filename() to test/update the systems table, rather than a db_query().

I think that the test is in the correct place however, as it will only be called on admin/modules, and only for moved modules. I don't think the performance hit in this situation makes it worth the extra code to push multiple changes into a single query.
I can't find the file_exists() Steven mentioned.

I will try to have a look at patching this now

OK scratch the part about using drupal_get_filename(), I misread the docs - it only updates the value for the current session.

I did find another location where the fix could fit in quite nicely (see attached patch) - in system_modules(), right before the module is loaded. The only benefit of this over the other patch, is that it seems less likely to duplicate updating, because the themes code already updates the location for each theme after calling system_get_files_database() - which would (I think) cause any moved modules to get updated twice.

The other possibility for fixing this is (which I think is the part Steven is referring to) would be for drupal_get_filename() - right after the file_exists() check - to update the database. The problem with this is I don't know if this is really part of this functions spec or not - and also that this code does not seem to be running when a module is moved.

That works, too. I haven't tested it, but it looks fine.

Is there any reason why for themes and theme engines we wipe out and recreate the system table each time, but for modules, we update it? Shouldn't we be doing the same thing (either delete+insert or update) for all three?

This I have found an excellent piece of code. Very short, to the point and even watchdogs.

Tested the patches in both comment #20 and #24. #24 didn't seem to recognize when I'd moved a file from a site-specific directory to the overall drupal/modules folder, while #20 handled it flawlessly.

this makes rolling out tests sites a LOT easier, as themes/modules will automatically adapt to a new directory. so, a BIG +1-this-works to the #20 patch. (system_module_moving_files_r2.patch)

A reroll of the patch in proper Drupal format.

I can confirm that patch #24 does not work for the instance of moving a module to a sites/*/modules directory. I think this must be related to the use of drupal_get_filename to test if the file has moved. Maybe testing for false when compared to $file->filename will catch this case?

Here is a working patch. It adds one line of new code ($file->oldfilename), and modifies 3 others.

The advantage of this over the patch in #20 is that:
* It doesn't duplicate updates when called from theme code.
* It adds a $file->oldfilename variable, which could be used in another patch to make system_theme_data() update the system table as neccessary, rather than delete & insert everything as it does now).

The advantage over patches #24 and #29 are:
* That it works for situations where the folder is either renamed, moved between /modules <--> /sites/*/modules or both renamed and moved.

There is no watchdog call (the update is invisible to the user), but I don't know how necessary that is, especially if the system works reliably!

Now, this one is real good, no surplus queries and yes it works -- sorry for mine had issues.

Yup. Tested here on the same site I've tested all the others on - it works fine and dandy.

Oh, and with regards to watchdog msg or not, I'd say its unneccessary. In most cases it will be the site maintainer who moves themes and modules files around, so Drupal should just deal with it quietly. Nice one, all.

committed. change oldfilename to old_filename.

This revised patch should fix an issue where new modules are no longer detected

Here is a patch on HEAD

the last conditions weren't right. here's one that should do it

Tested against CVS. This works when a module is moved, and also doesn't break when a module is added.

same thing, i just had an extra set of parentheses that i needed to remove :)

applied.