Spun off from #538904: D8UX: Redesign Modules Page
Problem/Motivation
It's really hard to find modules that were recently added to my site, when viewing the module list page.
Proposed resolution
Add the 'mtime' index to module and theme info arrays in the system table. mtime will contain the timestamp when the module/theme's .info was last saved.
Remaining tasks
- backport to D7
User interface changes
There will be no UI changes in core. Adding mtime will allow contrib modules to do things like sort the modules page by most recently-added modules.
API changes
- ?
Comment | File | Size | Author |
---|---|---|---|
#73 | 1355526-info-mtime-73.patch | 3.05 KB | cafuego |
#73 | 1355526-info-mtime-73-testonly-mustfail.patch | 1.95 KB | cafuego |
#69 | 1355526-info-mtime-69.patch | 3.05 KB | jenlampton |
#67 | 1355526-info-mtime-67.patch | 3.05 KB | cafuego |
#67 | 1355526-info-mtime-67-testonly-mustfail.patch | 1.95 KB | cafuego |
Comments
Comment #1
cafuego CreditAttribution: cafuego commentedThe 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.
Comment #2
cafuego CreditAttribution: cafuego commentedProof of concept patch attached. Uses a separate module form for the date-ordered modules.
Comment #3
theborg CreditAttribution: theborg commentedLooks good to me, I was working on loading the date from watchdog files but this patch, indeed, is more flexible.
Comment #4
cafuego CreditAttribution: cafuego commentedI 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.
Comment #5
jenlampton@cafuego the page needs to do a SQL query to find out which modules are enabled, so I'm sure we could retrieve the added date from that query if we had to (not sure about the update/insert part). But, I like this approach better. :)
Now all we need is a UI for these new modules...
Comment #6
cafuego CreditAttribution: cafuego commented@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.
Comment #7
jenlampton@cafuego yeah, I'd like a little more guidance for the UI. I personally think today/yesterday/this week/earlier is way too much. But it will be interesting to see what comes out of #538904: D8UX: Redesign Modules Page
Comment #8
cafuego CreditAttribution: cafuego commentedFair enough. I added those based on my own workflow, so they're possibly not useful to everyone else ;-)
Comment #9
cafuego CreditAttribution: cafuego commentedOkay, 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.
Comment #10
cafuego CreditAttribution: cafuego commented*cough* Lets try that again after some coffee and add the mtime to the correct array element :-)
Comment #11
Bojhan CreditAttribution: Bojhan commentedIs it possible to see, the effect of this?
Comment #12
cafuego CreditAttribution: cafuego commentedThis patch by itself has no visible effect, all it does is add an element to an array, which is then as yet unused internally.
Comment #13
Bojhan CreditAttribution: Bojhan commentedOk, 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?
Comment #14
xjmThis seems harmless to me offhand. Let's add a test for it?
Comment #15
cafuego CreditAttribution: cafuego commentedWhat do you want to test? That the call to load .info files returns the mtime element?
Comment #16
cafuego CreditAttribution: cafuego commentedModified comment slightly and added a test to ensure the system.info mtime field is present and contains likely looking data.
Comment #17
cafuego CreditAttribution: cafuego commentedTest that patch with only test and no functionality in fact fails, epically!
Comment #19
klonos...why was the the t() function wrap removed from the messages in the second patch?
Comment #20
cafuego CreditAttribution: cafuego commentedModified 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.
Comment #21
cafuego CreditAttribution: cafuego commentedOops, make bot test the patch.
@klonos As per the comment, because @xjm said so ;-) See: #500866: [META] remove t() from assert message.
Comment #22
klonosCool. Thanx.
Comment #24
cafuego CreditAttribution: cafuego commentedFlattened patch with the code change *and* latest version of the tests. This should be the one.
Comment #25
xjmI 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?
Comment #26
cafuego CreditAttribution: cafuego commentedI 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?
Comment #27
cafuego CreditAttribution: cafuego commentedRerolled the patch, checking whether the mtime is present, numeric and greater than 0.
Comment #28
jenlamptonThis looks great :)
Comment #29
chx CreditAttribution: chx commentedMake 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.
Comment #30
jenlamptonAgreed, when we add something to the UI that indicates "what's new" we should label it carefully :)
Comment #31
yoroy CreditAttribution: yoroy commentedThis 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.
Comment #32
Bojhan CreditAttribution: Bojhan commentedI am going to assign this to catch for commit, its passed its RTBC waiting time.
Comment #33
catchHmm 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?
Comment #34
cafuego CreditAttribution: cafuego commentedThat 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() :-)
Comment #35
cafuego CreditAttribution: cafuego commentedUpdated 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).
Comment #36
cafuego CreditAttribution: cafuego commentedComment #37
cafuego CreditAttribution: cafuego commentedDear bots, please retest.
Comment #38
cafuego CreditAttribution: cafuego commentedLets try that again with the tests on the inside of a class. *brown paper bag*
Comment #40
cafuego CreditAttribution: cafuego commentedJust 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.
Comment #41
kscheirer#40: 1355526-info-mtime-40.patch queued for re-testing.
Comment #43
cafuego CreditAttribution: cafuego commentedRerolled patches for new modules.test file location.
Comment #45
cafuego CreditAttribution: cafuego commentedRe-attaching the full patch only, so the bot doesn't change the issue status back.
Comment #46
kscheirerFrom #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.
Comment #48
xjmAgreed 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.
Comment #49
catchYep, looks good now. Committed/pushed to 8.x.
Comment #50
cafuego CreditAttribution: cafuego commentedLovely, 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!
Comment #53
cafuego CreditAttribution: cafuego commentedI see what you did there...
Comment #54
cafuego CreditAttribution: cafuego commentedModifying the tags.
Comment #55
Bojhan CreditAttribution: Bojhan commentedComment #56
xjmYep, looks backportable to me too.
Comment #57
David_Rothstein CreditAttribution: David_Rothstein commented#53: 1355526-info-mtime-51.patch queued for re-testing.
Comment #58
David_Rothstein CreditAttribution: David_Rothstein commentedPatch 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:
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.
Comment #59
webchickThat 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.
Comment #60
cafuego CreditAttribution: cafuego commentedHow about this instead?
Comment #61
David_Rothstein CreditAttribution: David_Rothstein commentedSomething 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?
Comment #62
cafuego CreditAttribution: cafuego commentedWow, I'd totally lost track of this patch! Attached is a patch for D8 that changes the code comment as per comment #60.
Comment #63
cafuego CreditAttribution: cafuego commentedD'oh, forgot the comment in _system_rebuild_theme_data(). Updated patch attached.
Comment #64
Bojhan CreditAttribution: Bojhan commentedBack to RTBC
Comment #65
catchCommitted/pushed the follow-up to 8.x, moving back to 7.x again.
Comment #66
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedComment #67
cafuego CreditAttribution: cafuego commentedFull D7 patch with functionality, tests and updated comments attached, as well as a test-only patch that should fail.
Comment #69
jenlamptonRe uploading test-passing patch (with no changes), and changing status back to RTBC!
Comment #70
jenlamptonOr maybe I need to do that in two steps, got a little too excited :/
Comment #71
cafuego CreditAttribution: cafuego commented69: 1355526-info-mtime-69.patch queued for re-testing.
Comment #73
cafuego CreditAttribution: cafuego commentedThe patch no longer applies to HEAD, rejigging it.
Comment #75
mgifford73: 1355526-info-mtime-73.patch queued for re-testing.
Comment #76
mgiffordWas 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.
Comment #77
cafuego CreditAttribution: cafuego commentedThere is no associated UI change, it's up to contrib to do something with this data.
Comment #78
mgiffordOk, so it can be marked RTBC with a code review & when there's a confirmation that the system table now includes a date column.
Comment #79
dcam CreditAttribution: dcam commentedThe 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.
Comment #82
dcam CreditAttribution: dcam commentedComment #85
dcam CreditAttribution: dcam commentedComment #90
dcam CreditAttribution: dcam commentedComment #95
dcam CreditAttribution: dcam commentedComment #98
dcam CreditAttribution: dcam commentedComment #101
dcam CreditAttribution: dcam commentedComment #104
dcam CreditAttribution: dcam commentedComment #107
dcam CreditAttribution: dcam commentedComment #110
dcam CreditAttribution: dcam commentedComment #111
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks!