Add a way to determine the date a module was added so the modules page can use it for sort

The module page does not use an SQL query to sort its output as far as I can tell, rather it builds an array by scanning the filesystem for .info files and then performs an alpha sort on the results.

To list the date a module was added to the filesystem (as opposed to enabled) you don't need an extra column on the system table. Since a module's .info file is opened and parsed when the module list is created, it's only a minute amount of overhead to get the last modified time for the same file as well.

This requires a call to filemtime() in core/include/common.inc:drupal_parse_info_file() and for the resulting timestamp to be added to the info array for the module filename. At that point, it'll automatically be saved in the serialized module info field in the system table.

To display recently installed modules, either the existing system_modules() form needs to be modified or a new modules form needs to be defined. Modifying the existing function is pretty straightforward, but would make it less readable.

Proof of concept patch attached. Uses a separate module form for the date-ordered modules.

Looks good to me, I was working on loading the date from watchdog files but this patch, indeed, is more flexible.

I should probably also mention that I've had a try of adding an mtime column to the system table, but then found that the updated INSERT call actually tries to run before the update hook that adds the field. Oops! So if it is to be done, that's slightly trickier than it may appear to be at first glance.

@jenlampton Do you want to let ideas for the UI come from the research that's to come out of #538904: D8UX: Redesign Modules Page ?

The patch attached above has a very basic grouping of modules added today/yesterday/this week/earlier, that work precisely like the groupings on the normal modules page. Though it does order by date added, it doesn't display it.

Fair enough. I added those based on my own workflow, so they're possibly not useful to everyone else ;-)

Okay, lets get this show on the road with a tiny patch.

The attached patch adds the .info file mtime to the array returned by drupal_parse_info_file(). With that extra field, it's already possible to implement a date-sorted modules admin page as a contrib module.

The patch is also small and innocuous enough that it can be ported to and perhaps allowed in D7.

*cough* Lets try that again after some coffee and add the mtime to the correct array element :-)

Is it possible to see, the effect of this?

This patch by itself has no visible effect, all it does is add an element to an array, which is then as yet unused internally.

Ok, I suggest we do not introduce the UI elements in this issue - but rather focus on enabling us to do it.

Can this get a code review?

This seems harmless to me offhand. Let's add a test for it?

What do you want to test? That the call to load .info files returns the mtime element?

Modified comment slightly and added a test to ensure the system.info mtime field is present and contains likely looking data.

Test that patch with only test and no functionality in fact fails, epically!

...why was the the t() function wrap removed from the messages in the second patch?

Modified the test slightly to not throw a notice if the field we're testing for is not present. Attached patch should fail two tests and not throw exceptions.

Oops, make bot test the patch.

@klonos As per the comment, because @xjm said so ;-) See: #500866: [META] remove t() from assert message.

Cool. Thanx.

Flattened patch with the code change *and* latest version of the tests. This should be the one.

I was about to RTBC this, but on re-read I doubted whether we can make the assumption that core has just been modified when the test runs. And, indeed, the test fails locally on my existing D8 installation. Thoughts?

I guess I can pick an arbitrary date and test that the system.info file mtime is set and is newer than that. I've redone the patch and used the D7 release date as timestamp to test against. You reckon that idea will fly?

Rerolled the patch, checking whether the mtime is present, numeric and greater than 0.

Adding a date column to the system table will enable us to add a "new" or "recently installed" section to the modules page, making the process of finding that module you *just* added easier to locate on the page.

Make sure you don't call it "recently installed" because what this patch does is allow you to see files "recently copied to the server". They are not installed. They are just there.

Otherwise, yeah, not really a better indicator for finding the stuff I just ftp'd up so let's go ahead with committing this.

This has my +1 too. At the D7UX sprint in 2009 we came up with the idea of having a "new" box on top of the modules page. It'd be great to be able to do that for D8.

I am going to assign this to catch for commit, its passed its RTBC waiting time.

Hmm I don't think we should put this in the drupal_parse_info_file() function, that's supposed to be a straight format parser, adding the mtime in there doesn't seem right. What about in _system_rebuild_module_data() instead?

What about in _system_rebuild_module_data() instead?

That would work, but as far as I can tell that means theme .info files miss out.

Edit: Unless I also patch _system_rebuild_theme_data() :-)

Updated patch as per comment 33. The .info file mtime is now added in _system_rebuild_module_data() and _system_rebuild_theme_data() instead.

Attached are a full patch (which should pass) and a test-only patch (which should fail).

Dear bots, please retest.

Lets try that again with the tests on the inside of a class. *brown paper bag*

Just changed once instance of 'system' to 'bartik' in a test comment compared to the patch on #38. Should all work now and be ready for a possible RTBC again.

#40: 1355526-info-mtime-40.patch queued for re-testing.

Rerolled patches for new modules.test file location.

Re-attaching the full patch only, so the bot doesn't change the issue status back.

From #12 and #13, this patch has no visual effect, just setting the stage for making use of this in the UI. Testbot is happy, looks like a good addition to me.

Agreed on the RTBC (note that "testbot is happy" isn't the only requirement). :) I've reviewed #45, and both the feature and the test for it look complete to me.

Yep, looks good now. Committed/pushed to 8.x.

Lovely, thanks catch.

Because it's immediately usable by contrib, doesn't break anything and webchick allows minor changes to D7, I've backported it. The changes to system.module are identical as the 8.x patch. The tests live in a slightly different location in system.test. Patches attached, bombs away!

I see what you did there...

Modifying the tags.

Yep, looks backportable to me too.

#53: 1355526-info-mtime-51.patch queued for re-testing.

Patch looks reasonable, but we're not under thresholds currently, so I'm not committing it to D7 yet. Hopefully soon... sooner if you help with the major/criticial queue :)

However, while I was looking at the patch, I found the comments a little confusing:

+    // Add the info file modification time, so the modules page can be sorted
+    // by the date modules were added or updated.

To me, that makes it sound like Drupal is actually sorting the modules page that way, but rather we're just adding the data here and that's one possible way the data could be (theoretically) used.

Might be useful to clarify that somehow.

That sounds like a needs work.

Additionally, David has requested help on resolving release blockers, so I don't feel comfortable committing 7.x feature patches until that happens.

How about this instead?

// Add the info file modification time, so it becomes available for
// contributed modules to use for ordering module and theme lists.

Something like that sounds good to me.

So I guess if we're going to change the code comment, we should probably get that into Drupal 8 first and then backport the whole thing at once?

Wow, I'd totally lost track of this patch! Attached is a patch for D8 that changes the code comment as per comment #60.

D'oh, forgot the comment in _system_rebuild_theme_data(). Updated patch attached.

Back to RTBC

Committed/pushed the follow-up to 8.x, moving back to 7.x again.

Full D7 patch with functionality, tests and updated comments attached, as well as a test-only patch that should fail.

69: 1355526-info-mtime-69.patch queued for re-testing.

The patch no longer applies to HEAD, rejigging it.

73: 1355526-info-mtime-73.patch queued for re-testing.

Was the UI change ever implemented? I was expecting to see a date column here /admin/modules or at least some screenshots.

I assume that this just adds a date column to the system table. Just making sure I know what to test for.

There is no associated UI change, it's up to contrib to do something with this data.

Ok, so it can be marked RTBC with a code review & when there's a confirmation that the system table now includes a date column.

The code review looks good.

There will not be a new column added to the system table. See #12. The Proposed Resolution section of the issue summary was incorrect. I've updated it. Instead of adding a new column, the date is put into the 'mtime' index of the serialized array in the info column.

I applied the patch and now I have 'mtime' in my module/theme info data. RTBC.

Committed to 7.x - thanks!