QM should respect `DISALLOW_FILE_MODS`

[JulienMelissas]

We use a custom db.php drop-in with a few things we need in production (like connecting to a DB on GCP with certs). We also use DISALLOW_FILE_MODS hoping that most plugins won't make changes to files on the disk if that flag is set to true.

We have encountered this twice now where we'll deactivate QM on a sub site and it'll delete the db.php file completely, bringing down all of the sites on our network. For now we are removing the plugin just in case someone forgets or accidentally disables. It would be great if db.php wasn't deleted if DISALLOW_FILE_MODS is set to true.

Let me know if you need any other information, thank you!

[crstauf]

Interesting... the code associated with deleting db.php should be checking it belongs to QM before deleting it:

https://github.com/johnbillion/query-monitor/blob/6ca0585108306473bad233f87c570e65a11ceb6f/classes/Activation.php#L58-L61

I agree that QM could respect DISALLOW_FILE_MODS in general, but it's curious that db.php is being removed given the logic surrounding the function. Could mean something else is happening.

[johnbillion]

I agree that QM should respect DISALLOW_FILE_MODS by default, I'll add that to my to-do list.

However as Caleb said, it should only get deleted if the file belongs to QM in the first place. Does your custom db.php file include a class named QM_DB?

[MadtownLems]

Crazy timing! I came to look up a different issue and was curious about this one. I've been trying to understand why my db.php symlink kept going away, and it is exactly the case that deactivating it on a single site in multisite removes the symlink (and yes we have DISALLOW_FILE_MODS in place).

For reference, I've been manually creating the symlink per the instructions here: https://github.com/johnbillion/query-monitor/wiki/db.php-Symlink

[johnbillion]

@JulienMelissas @MadtownLems In the next release, QM won't attempt to create the db.php dropin during activation if DISALLOW_FILE_MODS is true, and it won't attempt to delete it during deactivation on a single site within a Multisite network.

It doesn't specifically check DISALLOW_FILE_MODS during deactivation. I think it's safe to say that during deactivation on a single site installation or when network-activated on a Multisite installation, it's preferable to allow QM to attempt to delete the db.php file. Let me know if you can think of a reason why that might not be the case.

Cheers!

[JulienMelissas]

Hey @johnbillion - I'm so sorry for not replying back sooner, it turns out GitHub defaulted to sending any follow-up email issues to my personal address which I've been particularly bad at checking this month 😅

To answer the earlier question, no our db.php does/did not have any class named QM_DB in it 🤔

I really appreciate the code change made and your reasoning makes sense for the first bit, but I personally still feel that if DISALLOW_FILE_MODS is true/truthy, no plugins should make any file changes.

In our case, the subsite deactivation check would fix the individual deactivation problems we saw, but if the plugin was network activated (maybe in a testing environment or something) and then deactivated, it would still bring down the network on file deletion since we have other things in the db.php file that allow us to connect to the staging DB. Thanks for your consideration! And for the plugin, of course.

[crstauf]

This probably should be re-opened to diagnose db.php being deleted when QM_DB class does not exist.

[crstauf]

@JulienMelissas Can you please do one of the following:

1. Search your codebase for class QM_DB
2. Print out the result of var_dump( class_exists( 'QM_DB' ) )

As mentioned previously, QM should only delete db.php if QM_DB exists, so we need to rule out that it does exist somewhere.

If any plugins or packages have QM as a dependency, could that be loading QM_DB?

[JulienMelissas]

(Keep in mind this is after uninstalling Query Monitor, we use composer to manage our plugin dependencies)

1. No results:

   

2. Also nope (var_dump( class_exists( 'QM_DB' ) ); die; was added to the top of my theme's index.php file):

   

To your last question, we do have plenty of plugins, but I can't think of any that might be loading in QM as a dependency. Let me know if you have any other questions!

[crstauf]

:scratching_head:

[JulienMelissas]

I mean, wouldn't it evaluate as true if the plugin was active on the sub site right before the plugin was deactivated? Or do I have the order of operations wrong here?

[crstauf]

If QM's database drop-in is not present, then the file shouldn't be deleted. That's what the class_exists( 'QM_DB' ) check does.

[crstauf]

@johnbillion Do you think evaluating the contents of db.php would be more reliable?

$filepaths = array(
    constant( 'WP_CONTENT_DIR' ) . '/db.php',
    $this->plugin_path( 'wp-content/db.php' ),
);

$file_contents = array_map( 'file_get_contents', $filepaths );

if ( $file_contents[0] === $file_contents[1] ) {
    unlink( $filepaths[0] );
}

Something similar is done here:

https://github.com/johnbillion/query-monitor/blob/2c7107cd7e603028d52e9e5c2dd263f383a73b07/classes/CLI.php#L29-L40

Or perhaps something more simple, like checking for @package query-monitor in the drop-in?

[JulienMelissas]

Right, so the issue we had is:

1. db.php is used on a WP multisite for non-QM things (and had no QM code in it when pushed up to production)
2. QM was active on a subsite
3. QM was deactivated on a subsite
4. db.php file was removed from the filesystem

I'm looking at these lines right now and wondering if somewhere between 2 and 3 class_exists( 'QM_DB', false ) resolves as true, even though the plugin is technically deactivated?

[crstauf]

Seemingly the only path to QM_DB class being defined is the database drop-in (wp-content/db.php).

Is setting permissions on the file to be read-only an option? I'm not very knowledgeable in such things, so might not be an option or solve the problem.

[JulienMelissas]

Is setting permissions on the file to be read-only an option? I'm not very knowledgeable in such things, so might not be an option or solve the problem.

On some hosts this is possible, for sure, but in our case we don't have access to make changes to file access in production with something like chmod.