Posted by jenlampton on November 29, 2011 at 8:39am
17 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | base system |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | API addition, needs backport to D7, Usability |
Issue Summary
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
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.
Remaining tasks
- make this happen
User interface changes
- create a "new" or "recently added" section to the top of the modules page
API changes
- ?
Comments
#1
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.
#2
Proof of concept patch attached. Uses a separate module form for the date-ordered modules.
#3
Looks good to me, I was working on loading the date from watchdog files but this patch, indeed, is more flexible.
#4
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.
#5
@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...
#6
@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.
#7
@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
#8
Fair enough. I added those based on my own workflow, so they're possibly not useful to everyone else ;-)
#9
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.
#10
*cough* Lets try that again after some coffee and add the mtime to the correct array element :-)
#11
Is it possible to see, the effect of this?
#12
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.
#13
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?
#14
This seems harmless to me offhand. Let's add a test for it?
#15
What do you want to test? That the call to load .info files returns the mtime element?
#16
Modified comment slightly and added a test to ensure the system.info mtime field is present and contains likely looking data.
#17
Test that patch with only test and no functionality in fact fails, epically!
#18
The last submitted patch, 1355526-17.patch, failed testing.
#19
...why was the the t() function wrap removed from the messages in the second patch?
#20
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.
#21
Oops, make bot test the patch.
@klonos As per the comment, because @xjm said so ;-) See: #500866: [META] remove t() from assert message.
#22
Cool. Thanx.
#23
The last submitted patch, 1355526-19.patch, failed testing.
#24
Flattened patch with the code change *and* latest version of the tests. This should be the one.
#25
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?
#26
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?
#27
Rerolled the patch, checking whether the mtime is present, numeric and greater than 0.
#28
This looks great :)
#29
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.
#30
Agreed, when we add something to the UI that indicates "what's new" we should label it carefully :)
#31
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.
#32
I am going to assign this to catch for commit, its passed its RTBC waiting time.
#33
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?
#34
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() :-)
#35
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).
#36
#37
Dear bots, please retest.
#38
Lets try that again with the tests on the inside of a class. *brown paper bag*
#39
The last submitted patch, 1355526-info-mtime-38-testonly.patch, failed testing.
#40
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.
#41
#40: 1355526-info-mtime-40.patch queued for re-testing.
#42
The last submitted patch, 1355526-info-mtime-40.patch, failed testing.
#43
Rerolled patches for new modules.test file location.
#44
The last submitted patch, 1355526-info-mtime-43-testonly.patch, failed testing.
#45
Re-attaching the full patch only, so the bot doesn't change the issue status back.
#46
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.
#48
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.
#49
Yep, looks good now. Committed/pushed to 8.x.
#50
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!
#51
The last submitted patch, 1355526-info-mtime-50.patch, failed testing.
#53
I see what you did there...
#54
Modifying the tags.
#55
#56
Yep, looks backportable to me too.
#57
#53: 1355526-info-mtime-51.patch queued for re-testing.
#58
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.
#59
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.
#60
How about this instead?
#61
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?
#62
Wow, I'd totally lost track of this patch! Attached is a patch for D8 that changes the code comment as per comment #60.
#63
D'oh, forgot the comment in _system_rebuild_theme_data(). Updated patch attached.
#64
Back to RTBC
#65
Committed/pushed the follow-up to 8.x, moving back to 7.x again.
#66
#67
Full D7 patch with functionality, tests and updated comments attached, as well as a test-only patch that should fail.
#68
The last submitted patch, 1355526-info-mtime-67-testonly-mustfail.patch, failed testing.
#69
Re uploading test-passing patch (with no changes), and changing status back to RTBC!
#70
Or maybe I need to do that in two steps, got a little too excited :/