Problem/Motivation
There needs to be an addition to the Drupal Admin UI to give users a way to browse through Drupal Modules (along with summary, user stats, screenshot) from within Drupal. The idea is to replicate the WordPress UI and make it more obvious to new users of Drupal that they are able to extend Drupal's basic install.
Proposed resolution
Incorporating Project Browser into Drupal 8 Core. This will make it much more obvious to new users of Drupal that they are able to extend the functionality of Drupal through adding modules. It will also make the process of installing and enabling these modules more intuitive to new users.
Remaining tasks
User interface changes
This feature allows projects (modules) to be viewed and searched within the Drupal UI.
API changes
API changes are yet to be documented.
Original report by wildkatana
As a culmination of a GSOC project from back in 2011, I am submitting this patch to get Project Browser (http://drupal.org/project/project_browser) put into Drupal 8 Core.
It is working on Drupal 8 and Drupal 7. It will be available as a contrib for Drupal 7, but we want to get it into Drupal 8 Core so that users will have an easy way to browse for and install modules straight from their admin pages, similar to how WordPress currently does it.
Recent Demo Video: http://www.youtube.com/watch?v=ddC7M6sy5lA
D8 Demo Site (click 'Install new modules'): http://d8.leightonwhiting.com/admin/modules (admin/test)
This was made in large part to address #1164760: [meta] New users universally did NOT understand that you can extend Drupal and because one thing that I've had to explain to new Drupal users over and over is how to install a module, and what a module is. WordPress has had this functionality for years and it is a HUGE help to new users who simply don't know how to use FTP or how to install or even find modules to extend their platform.
The original design was based on Bojhan Somer's blog post here: http://bojhan.nl/module-theme-browser-interface
Related Issues:
#1243332: Deploy Project Browser Server and drupalorg_pbs on d.o
Currently it is running the server module from http://pbs-drupal_7.redesign.devdrupal.org but there is an issue to get it deployed to Drupal.org. Once it is deployed to drupal.org, we can change the server url in the Project Browser code to point to the correct place.
Currently there are some bugs in the 7.x implementation of Project Browser server because there are some ApacheSolr issues that don't allow for filtering out Drupal core and differentiating between Modules and Themes. These bugs are not in the 6.x version which will hopefully be deployed soon. The 7.x bugs will be fixed once the project_solr module is fully working. Also, images aren't working yet with the 7.x implementation. Again, waiting for the images field to be ported to D7.
Comment | File | Size | Author |
---|---|---|---|
#176 | wordpress-plugin-browser.jpg | 139.28 KB | webchick |
#146 | drupal-project_browser-1841788-146.patch | 104.39 KB | douglasmiller |
#146 | interdiff-120-146.txt | 5.21 KB | douglasmiller |
Comments
Comment #1
webchickPatch? :)
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedHere it is.
Comment #3
yoroy CreditAttribution: yoroy commentedRTBC when it comes back green ;-)
Exciting!
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedLooks like it passed to me.
Comment #5
sunComment #6
sunI quickly glanced over this patch. It violates coding standards in many different ways.
In case this code lives in a sandbox, it would be helpful to grant access to people who'd like to help with polishing the code to get it ready. If it does not live in a core sandbox yet, then it should. ;)
One major thing that immediately stood out in my brief review: Why do we need the jQuery multiselect library? I have a lot of experience with these kind of libraries, and almost all of them are poorly designed and most often, ultimately unnecessary in times of HTML5.
Comment #7
yoroy CreditAttribution: yoroy commentedYeah, that rtbc was a joke, this needs some more review first :-) Awesome that it turns up green though.
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commented@sun - The jQuery multiselect was in there for D7 to make it easier to select the categories (see screenshot). We can get rid of it if needed. It didn't work in D8 and I haven't looked at why yet, but was planning to.
I've been using the 8.x branch of the Project Browser repository, but I think that a separate sandbox would be best, so I have created this new Sandbox here: http://drupal.org/sandbox/wildkatana/1842072 . I've added you as a maintainer so you can commit the changes. I am also still working on a couple bugs as well.
There are still some rough edges in the module and some problems caused by the D7 Dev Server (ApacheSolr isn't working all the way yet at filtering modules/themes), but I wanted to get this issue rolling to get it in before the Feature Freeze as noted by webchick in the other issues.
@yoroy - I was as surprised as anyone at the pass lol.
On a related note, this module makes use of some code from the update module. I would like to patch the update module to move that code to a function that can be called from both places instead of duplicating code. Should I open an issue with the patch once I get that changed?
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedAnd the screenshot
Comment #10
yannickoo@wildkatana you editor really should remove all the whitespaces on save and you also should use spaces instead of tabs. Why did you have tabs sometimes?
Comment #11
yoroy CreditAttribution: yoroy commentedBefore diving into code style issues it might be better if wildkatana could do a writeup of the architecture to help reviewers grok the flow of the code.
Comment #12
eigentor CreditAttribution: eigentor commentedI gave a quick test to the demo site http://d8.leightonwhiting.com/admin/modules
There are some things that are not so nice:
- The general formatting lacks formatting, hard to seperate controls from content. (Edit: watching the youtube video shows this is just the D8 version, the D7 version displayed there looks better) The placement of the controls also feels awkward.
- The search box (labeled "Search String") is behaving weird: it appears to search everything but the module names themselves. If I search for Webform, I get only modules mentioning webform in their description or maybe dependency, I don't know.
Look what comes up when searching for "Chaos":
http://screencast.com/t/iA5WVdao
The screenshot also illustrates the other issues mentioned.
What is nice:
- The idea with the install queue adds a nice "module shopping cart" that can be filled one by one, then you execute and get a coffee, cause the installing itself takes a while of course.
- After Installing, I get taken to a screen to enable the new modules, with only the just uploaded modules being listed. This provides a solution for the difficulty to find new modules after having uploaded them. It also provides a solution for the user not understanding he has to enable modules before they can be used.
For inclusion in core, though, I fear the module needs some serious IA and Design work (all coding issues aside). I am a bit pessimistic this will be possible so short before feature freeze.
But let's not shoot this down so soon, as it is a good initiative of wildkatana.
Comment #13
Anonymous (not verified) CreditAttribution: Anonymous commented@yannickoo - Yeah, sorry about that, I must have opened it in my windows IDE once. I'll convert those to Linux style.
@yoroy - I will go through and add lots and lots of comments in the code, that should help, right?
@eigentor - Yes, the D8 port isn't perfect yet, I still have more work to do on it but wanted to open the issue now to get the ball rolling. This was made in large part to address #1164760: [meta] New users universally did NOT understand that you can extend Drupal and because one thing that I've had to explain to new Drupal users over and over is how to install a module, and what a module is. WordPress has had this functionality for years and it is a HUGE help to new users who simply don't know how to use FTP or how to install or even find modules to extend their platform.
I am of course always open to design suggestions and want it to be as user friendly as possible. This is by no means a finished and done thing. I aim to continue to improve it. The original design was based on Bojhan Somer's blog post here: http://bojhan.nl/module-theme-browser-interface
In short I mean to say that I'm not married to the design, and anything that will make it better I will gladly embrace.
The search string is a result of the ApacheSolr index I think. It doesn't just search the title, it searches everything. See here on Drupal.org the same query, with the same results: http://drupal.org/project/modules/chaos?f[0]=drupal_core%3A7234&f[1]=bs_...
I am open to ideas on how to improve the accuracy of the search. I think it would be great for people who know the short name to just enter that and get it to find the correct module.
I think we need to get this into Drupal 8 core though to address #1164760: [meta] New users universally did NOT understand that you can extend Drupal, since it is a very important thing that will help push Drupal further into the main stream consumer market. As I said, it is not perfect yet, but it IS functional and it DOES fill a critical hole in Drupal usability for new or inexperienced users.
Comment #14
yannickooI just want to mention that the project name isn't in the url so that cannot work right?
Comment #15
nod_JS needs to be brought up to core standards. Lots of problems with it. I really want to set it to NW since so many easy coding standard issues are in the code.
Comment #16
webchickCoding standards and stuff is important to resolve before commit, but it would be great for more substantial reviews on the actual architecture/UX/etc. of the patch. wildkatana's worked on this for literally years.
Comment #17
Anonymous (not verified) CreditAttribution: Anonymous commented@yaninickoo - Not sure I follow, can you explain?
@nod__ Yeah the multiselect library was just something I dropped in to make it more user-friendly, so that JS code is pretty much copy/paste. It can be removed if needed. The other JS is old and done when I was less experienced. I'll go through it and clean it up.
I'm changing this to needs work until I fix that and other issues, and added more documentation. Then I'll move it to needs review again.
Comment #18
Anonymous (not verified) CreditAttribution: Anonymous commentedOkay, here's another patch with a number of resolved issues and more comments. Still more work to be done concerning the multiselect library. If this just needs to be removed, I will do it. Remember that the sandbox for this is here: http://drupal.org/sandbox/wildkatana/1842072 so feel free to pull it and make changes if you see things that need changing.
Comment #19
Bojhan CreditAttribution: Bojhan commentedI would be great to get some more reviews on this, it will be a great addition.
Comment #20
aspilicious CreditAttribution: aspilicious commentedI would love to get such a system in
But this is just to much. Please open your patch with dreditor to see the large amount of tabs and trailing whitespaces in your patch. It's distracting me from reviewing this patch.
Or try to configure your editor so it strips traling whitespaces ad converts tabs to 2 spaces.
Comment #21
andyceo CreditAttribution: andyceo commentedI reworked patch in #18 with #20 comment in mind.
Comment #22
andyceo CreditAttribution: andyceo commentedSorry, miss to rework project_browser.pages.inc.
Comment #23
Lars Toomre CreditAttribution: Lars Toomre commentedThanks @andyceo for cleaning up the white space issues.
This patch is also missing many trailing periods on most one-line summaries and @param/@return descriptions. Also this could use type hinting on all @param/@return directives.
Comment #24
aspilicious CreditAttribution: aspilicious commentedI made a coding style issue in the sandbox to keep this issue clean: http://drupal.org/node/1843446
Getting closer :)
Some thoughts:
1) Find a core js maintainer (maybe nod_ or seutje) to review the js as I think it's not yet adopted to the latest drupal8 js standards. (I can be wrong)
2) There are a few small templates in this patch. I would like to get an opinion about the theme system maintainers about this.
3) The multiselect doesn't work properly on my chrome
Comment #25
Anonymous (not verified) CreditAttribution: Anonymous commented@andyceo - Thanks for that, I think my editor (Aptana) was adding tabs when it auto-indented. Thanks for the dreditor suggestion, I can see those now. I guess I just need to change my IDE.
@aspilicious - Thanks for the issue, I'll go through it and start working through them.
About the multiselect not working, I am getting an error about jQuery.easing being undefined. I am not sure if this is because I am missing a dependency in the library or if this is a bug in Drupal 8, because I can clearly see the jQuery.easing IS defined just a few lines underneath the error point in jquery.js. Very strange...
Comment #26
Fabianx CreditAttribution: Fabianx commented#25: IIRC you need to declare libraries that you want to load now via #attached library property or so. That would explain the error.
Comment #27
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks to aspilicious for helping me get the module ready for core from a documentation standpoint #1843446: Coding style issues. After fixing a few bugs and CSS and JS issues, I am now resubmitting the patch here for review.
As per the suggestion in #26, I used the #attached property in the render array for the library and it has gotten rid of the error, but the multiselect still isn't working quite properly yet (at least in my browser). I'll keep working on that.
Also note that we are still using the staging server running Drupal 7, which isn't fully functional yet from a filtering/apachsolr perspective, which is messing up some of the results (like showing the Drupal project, and mixing modules and themes). These will all be fixed once those other issues get cleared up, and don't directly relate to the Project Browser module here, but rather the drupalorg_pbs module: http://drupal.org/project/drupalorg_pbs . Once we get the 6.x branch deployed on Drupal.org, those issues will go away: #1243332: Deploy Project Browser Server and drupalorg_pbs on d.o
Comment #28
Anonymous (not verified) CreditAttribution: Anonymous commentedForgot to change the issue status.
Comment #29
Lars Toomre CreditAttribution: Lars Toomre commentedThanks @wildkatana. The patch in #27 looks much, much better from a documentation perspective.
There were a few more recurring documentation items that I noticed when starting to review this patch. They include:
a) There are missing docblocks in some of the class files.
b) Coding standards call for class member variables to use $camelCase instead of $camel_case for example.
c) A blank line needs to be added after break; statements in switch blocks.
d) The correct format for items in a list is - example: A variable... There should be no space before colon and the explanation should start with a capital letter and end with a period.
e) The *_form_submit() one line function summaries should be like '* Form submisstion handler for *_form().'.
It would be great if you could address these in your next re-roll. Thanks.
Comment #31
Fabianx CreditAttribution: Fabianx commented#27 probably needs a re-roll due to the format of item_list being changed:
http://drupal.org/node/1842756
Comment #32
Anonymous (not verified) CreditAttribution: Anonymous commentedRe-rolling the patch with fixes from #29.
EDIT- Just saw the failed results, will get on those shortly and re-roll another patch in a couple hours.
Comment #33
Lars Toomre CreditAttribution: Lars Toomre commentedThanks @wildkatana. I appreciate the various updates.
When you next roll this patch can you create what is referred to as a squashed patch? I see that you like to use the git format-patch command. When I create such a squash patch, I create a temporary branch based on 8.x core and then 'git merge --squash' from the working branch. After committing the result, I then do something like 'git format-patch --stdout 8.x > ...' to generate the formatted patch. Hopefully, this abbreviated description helps.
The resulting patch is really easy to review since to all of the changes to each file are in the same place. Alternatively, one can do a 'git diff 8.x > ...' to create a patch from the working branch. Thanks in advance.
Comment #35
Anonymous (not verified) CreditAttribution: Anonymous commentedI'm not sure if I understand the squash merge process, but I gave it a try and am attaching the patch here. Fixed some things in the Test class causing the errors.
Comment #37
Anonymous (not verified) CreditAttribution: Anonymous commentedHere goes another try, after fixing the last issue with the data element.
Comment #38
falcon03 CreditAttribution: falcon03 commentedIs the demo site up to date with the latest patch?
If it does, then we have to address some accessibility issues to meet core standards (I'll write an accessibility review later)...
Comment #39
falcon03 CreditAttribution: falcon03 commentedfixing tags, sorry...
Comment #40
Anonymous (not verified) CreditAttribution: Anonymous commentedYes, it is running the latest D8 code and the latest project_browser patch.
Comment #41
falcon03 CreditAttribution: falcon03 commentedOk, maybe this is an accessibility & usability review, not a bare accessibility one. Here my suggestions are:
However I have a critical issue to focus on: the filter selection widget is not so easy to understand from a blind user perspective. It is announced as a "listbox", but when you try to select a value lots of checkboxes appear; this can be very confusing…
Also, another proposal: what about changing the workflow for adding a module to the "install cueue", turning the search results into a tableselect? Of course doing this will mean that the 5th point in the list I've written below shouldn't be considered anymore...
Comment #42
Anonymous (not verified) CreditAttribution: Anonymous commented@falcon03 - Thanks for the review, here are my comments:
Can you clarify what you mean here a bit? Which 'install new module' are you meaning? Is it the one on the Modules page that says "Install new modules"? I used an action link because it just replaces the old action link of "Install new module".
Showing the most common modules by default I think is a good thing, because it helps users who are new know what are some of the most popular modules they can install. This is how drupal.org does it (http://drupal.org/project/modules)
I can add a Sort By select box in the filters box to do this. Would that work?
I separated them because they are separated conceptually on drupal.org (http://drupal.org/project/modules and http://drupal.org/project/themes) but I can combine them and add a select box for type ('Modules' or 'Themes'). Just want to clarify that that is what is best.
Good point, done.
Are you meaning the place where it currently says "Modules" or "Themes" in the heading before the search results? I can change that to "Search Results", shouldn't be too hard.
This is caused by the multiselect checkbox, which I can see is becoming more of a pain to work with than it is worth. What if we made it so that users could only select one category at a time, and just use a simple select list for that? Or if we really want to keep the ability to select multiple categories, we could use the underlying html multiselect box without the jquery widget for it.
The main reason I don't like this is because the users are able to 'toggle' adding and removing the projects from the same link, and this can't be done with a tableselect. Also, some modules will already be installed and will still show up in the search results, but won't have the add to install queue link.
Comment #43
Bojhan CreditAttribution: Bojhan commentedCan you post screens etc, to review?
Comment #44
Anonymous (not verified) CreditAttribution: Anonymous commentedAttached is a screenshot of the main project browser page for modules. You can also go test it out here: http://d8.leightonwhiting.com/admin/modules/project-browser
Username is 'admin' and password is 'test'.
Comment #45
falcon03 CreditAttribution: falcon03 commented@wildkatana: here are my replies
You:
Can you clarify what you mean here a bit? Which 'install new module' are you meaning? Is it the one on the Modules page that says "Install new modules"? I used an action link because it just replaces the old action link of "Install new module".
Me:
Yes, I am referring to that one. Since this would be a more complete feature, I think that we should make it more visible than an action link is. What of these two approaches is better for sighted users? I wonder if having the "install new modules" as a primary tab could help new drupal users to understand that they can install new modules…
You:
Showing the most common modules by default I think is a good thing, because it helps users who are new know what are some of the most popular modules they can install. This is how drupal.org does it…
Me:
Ok, if they are the most common modules then you're right. But at this point I think that the search form should be moved on top of the most common modules and the "modules" title (in this situation) should become something similar to "most used drupal modules/themes". If I were a drupal user browsing the page, I think I would ask myself: "What have I searched for to see this list?"; having a clearer title and the search form on top of the list will help new users to answer this question, I think…
You:
I can add a Sort By select box in the filters box to do this. Would that work?
Me:
Yes, that's the solution that I was thinking about. This will allow us also to get rid of the "sort by" showed up with the search results.
About separating modules and themes: yes, you're right, they are conceptually separated on d.o. Honestly, I don't like very much this separation, especially when talking about a project browser with an install cueue just like this. It makes sense to have a "type" listbox on the search form, IMO.
Module names are properly wrapped into headings now, this is a big accessibility improvement. Great work!
You:
Are you meaning the place where it currently says "Modules" or "Themes" in the heading before the search results? I can change that to "Search Results", shouldn't be too hard.
Me:
Yes, I am referring to that one, sorry I weren't clearer. It makes sense to change it to "search results" when the search form has been submitted; otherwise it should be something similar to "most common drupal modules" (as I said before in this comment).
You:
This is caused by the multiselect checkbox, which I can see is becoming more of a pain to work with than it is worth. What if we made it so that users could only select one category at a time, and just use a simple select list for that? Or if we really want to keep the ability to select multiple categories, we could use the underlying html multiselect box without the jquery widget for it.
Me:
I vote up for the one category selection at a time with a simple select list. The default value of the list, however, should be "-- Any --", meaning that all categories will be searched for by default (the same happens with searching the issue cueue on d.o).
You:
The main reason I don't like this is because the users are able to 'toggle' adding and removing the projects from the same link, and this can't be done with a tableselect. Also, some modules will already be installed and will still show up in the search results, but won't have the add to install queue link.
Me:
OK, so let's go with your approach.
Great work, I hope we get this in core!
Comment #46
Anonymous (not verified) CreditAttribution: Anonymous commentedI think it would make sense as a tab, next to the List and Updates tabs on the Modules page. But I'm not sure if that would be acceptable for everyone, since the original implementation from the update module (that this one is replacing) is an action link. Are there any second opinions about this?
I agree with you here, having clearer titles makes sense. I'll see if I can get this worked in.
I was going to change this but then I thought about the sort direction. It seems we would need a sort direction select box as well as the sort type. The current widget lets you just change the sort direction by clicking on the sort type again, and it shows the current direction with a little arrow. I kind of like that and would rather use something like that than two select boxes. Any other opinions about this? Drupal.org seems to completely ignore the sort_direction, but I can see how it would be useful in some cases, like sorting by title.
If I can get a second opinion on this one as well, I will change it to combine the Modules and Themes pages into one and add a new 'type' filter select box.
I have changed it now to remove the multiselect. Should make this patch more manageable and accessible.
Comment #47
Anonymous (not verified) CreditAttribution: Anonymous commentedI've re-rolled the patch with some of the accessibility fixes, notably:
Removed the multi-select in favor of a single select box.
Used h2's for module titles
Changes the Listings heading to be 'Search results' when filters are used
There are still some unresolved accessibility items in #46 but I need some input from other Core developers about the best way to do those things marked in bold.
Comment #49
Anonymous (not verified) CreditAttribution: Anonymous commentedStrange. I'll try another pull and commit it again.
Comment #50
Anonymous (not verified) CreditAttribution: Anonymous commentedAnd changing the status
Comment #51
Anonymous (not verified) CreditAttribution: Anonymous commented#47: project_browser_accessibility_changes_1.patch queued for re-testing.
Comment #52
Lars Toomre CreditAttribution: Lars Toomre commentedI really like this proposed module and hope to see it in D8 core. Here are some notes from reading through the patch:
The standard for the @file docblock recently was changed. Now 'Definition of' should be 'Contains'.
Is there a follow-up issue opened for this @todo?
This member needs a docblock.
Variables are going away in D8 in favor of config and state sub-systems. Can we make the adjustment now?
I am unsure of what errors are being checked for in this method. Is the docblock incorrect?
In last few months, all t() around messages in test assert messages have been removed. The same should be done here.
Extra blank line and one line summary should start with 'Page callback:', like 'Page callback: Form constructor for admin settings.', since this function is called from *_menu().
Can we add 'array ' before $sort_options? Also elsewhere in this patch where an array is expected as function call variable type?
Here (and elsewhere in patch) the explanation should start with '(optional)' ir lower-case.
List documentation standard states that array keys like these should not be enclosed in quotes. Hence, '- version: '.
Can just be 'An array of results'. Also same list formatting comment as before.
Perhaps this value of '+24 hours' should be set via config file?
Seeing this, this value should be set via a config file. It definitely is not a state() value.
Has there been an issue filed to add this function to update module?
Missing '@throws \Exception' directive after blank line after @return directive.
Should be blank lines after each break;
Needs a period at end of line.
Comment #53
Anonymous (not verified) CreditAttribution: Anonymous commented@Lars: Thanks for the review, I have corrected all of them and have the following notes:
The documentation guidelines don't mention this, and the other core class files are still using the old method. Anyways, I changed it like you said :)
The exception isn't really thrown, it is actually caught in the method. Should I still document it even though it is handled?
I was holding off until after the feature freeze, but I've created one now: #1846078: Create a helper function that will install a project from local cache
EDIT: I accidentally left in a debug drupal_set_message, but have removed it already for the next patch.
Comment #54
Lars Toomre CreditAttribution: Lars Toomre commentedThanks @wildkatana. Could you provide an interdiff for what you changed?
re: @throws: I thought I saw a throw statement and that is why I made that note.
Thanks for opening up the follow-up issue.
Comment #55
Anonymous (not verified) CreditAttribution: Anonymous commentedI'm not entirely sure I did this right, but here is the interdiff
Comment #56
YesCT CreditAttribution: YesCT commentedI looked at the interdiff with dreditor and it doesn't look right.
Did you need help getting dreditor?
Sometimes what I do is upload the patch, then look at what I just uploaded with dreditor. So sometimes it I end up posting patches in rapid fire. But that's ok. :)
When I do interdiffs I use micro branching... ala xjm
http://xjm.drupalgardens.com/blog/interdiffs-how-make-them-and-why-they-...
http://drupal.org/node/1488712 doc for "Creating an interdiff"
Comment #57
Bojhan CreditAttribution: Bojhan commentedI think its fine to tweak the UI of this further after its in core, I can think of a few things we might want to convert to something more usable but thats all visual tweaking.
Could this get some more PHP reviews?
Comment #58
alexpottThis is just a review based in the config file.
cache_lifetime
value should be in seconds so that it is the same as other similar values in Drupal - this means that the cache->set() becomes simpler too :)Comment #59
Anonymous (not verified) CreditAttribution: Anonymous commented@alexpott - Thanks, here is the full patch with those changes.
Comment #60
alexpottLooks like these tests could do with some work as the
// Create node to edit.
seems to be a copy/paste error and the both are making the same assertion. For example, I would expect thetestProjectBrowserSearchViews
to assert that it has found a views related project and has not found other not views related projects.This needs to be getting the values from config.
There is no system_settings_form() any more - have a look at system_config_form() and you now need to write a submit handler to save this config.
Don't think you should be using $_SESSION here... do you not have the value in $form_state?
As above
Why doesn't this use Drupal's queue api? See http://api.drupal.org/api/drupal/core%21includes%21common.inc/function/q...
Shouldn't this be using the cache_lifetime config?
Should be using config()
maintenance_mode has been converted to config()
Newline needed
Should be using config... I'm also skeptical that $_SESSION being used here.
Why are we using $_SESSION here when you're adding an operation to the batch for each project?
A couple of other points from my quick scan of the code... the listing, searching, filtering functions should all really be leveraging views functionality as views in core! Which brings me to my second point... having the views module in the project_browser_test module is kind of redundant as views is in core!
Comment #61
alexpottQuick work - though you've still got unnecessary double quotes in the config file.
Comment #62
Anonymous (not verified) CreditAttribution: Anonymous commented@alexpott - Thanks for the detailed review! I made a number of fixes to the config and test systems.
I don't think I can use Views here because the results aren't being pulled from the database, but rather from a server via JSON.
The Queue API seems overly complex for the purposes of this module, which basically just needs to keep track of which projects the user has selected for installation.
The update module uses SESSION to keep track of the maintenance_mode, so that was why I used it there.
The filter values aren't available in the $form_state variable, and since the values were being stored in $_SESSION anyways (so that the form would 'remember' what the users searched for last), I just used those. If we want to get away from using $_SESSION here, I think the project browser results page is going to have to be built as a part of the filters form function, which would require a lot of refactoring...
Attached is the patch.
Comment #63
Anonymous (not verified) CreditAttribution: Anonymous commentedAnd here is the diff.
Comment #65
YesCT CreditAttribution: YesCT commentedThese get @params now.
(there are a bunch already in core that don't have them yet. but anywhere they are changed or added in new places, they get them.
Something like:
+ * @param array $form
+ * An associative array containing the structure of the form.
+ * @param array $form_state
+ * A reference to a keyed array containing the current state of the form.
* Downloads and installs a project using the update module.
tells better what this function does.
check I think this needs a period at the end. Might have a character limit.
I'm not sure where the doc on core info files is... I looked at
http://drupal.org/node/542202
I took out the repetitive A Project Browser that...
and reference to "their Drupal site"
no quotes needed here I think.
... I found a bunch of info files, using different patterns.
So I looked at something added to core recently that I thought would have been reviewed.
core/modules/views/tests/views_test_config/views_test_config.info
the other tpl.php files in this patch don't have this blank line.
extra blank?
I dont think it's written down anywhere for tpl. and if it is really needed/desired, then standardizing blank lines in project browser tpl files can be a follow-up.
actual question about needing @var for this $class variable in a tpl file
does this need a @var?
maybe missing newline.
I'll fix.
summary
I dont think any of this was blocking. I did not read through the whole patch in detail. Just fixed what I saw.
Adding the @params for form and from_state might make sense as a seperate interdiff as there are a lot of them.
dreditor and applying the patch warns of white space, but it's talking about the png files. So I guess that should be ignored.
Comment #66
YesCT CreditAttribution: YesCT commentedsorry, should still be needs work for the test failures in #62
Comment #67
Anonymous (not verified) CreditAttribution: Anonymous commented#62: pb_in_core_20.patch queued for re-testing.
I'm not sure what those errors mean... Any ideas? The tests pass on my dev server...
Comment #68
Anonymous (not verified) CreditAttribution: Anonymous commented#53: project_browser_config_etc_1.patch queued for re-testing.
Comment #69
Anonymous (not verified) CreditAttribution: Anonymous commentedHmm a previously working patch (#53) now has those same errors after a retest. Something must have changed in the testing system. Any ideas? I'll keep poking around and see what I can figure out.
Comment #70
eiriksmJust tested the patch and it calls undefined functions (i.e update_manager_install_project() and drupal_get_path_alias()) so the patch is not working at all on my computer.
That being said, i would love to see this in core. Keep up the good work!
Comment #71
Anonymous (not verified) CreditAttribution: Anonymous commentedThe update_manager_install_project problem is because this patch hasn't been applied to core yet: #1846078: Create a helper function that will install a project from local cache
The drupal_get_path_alias function can just be removed safely, since the current_path() function is used now.
Should I just use the old method in this issue until that is applied, or should I continue using the new method?
I still can't duplicate those errors in the patches above, nor do I know what they mean. I'll go through #65 and get those things fixed in a new patch tomorrow.
Comment #72
YesCT CreditAttribution: YesCT commentedProvide two patches, one that depends on that issue, and another that is independent.
Give instructions, to people who are testing that they will have to also apply the other patch as well.
Comment #73
YesCT CreditAttribution: YesCT commentedregarding my comment in #65 about needing @params for form and form_state, I learned some more about 1354 in #963518-40: Validate that private files aren't the same directory as public files
So I could take a closer look before any changes for form params are done.
Comment #74
Lars Toomre CreditAttribution: Lars Toomre commented@YesCT - Can you add a link that points to $form and $form_state now require @param directives? I do not see that requirement in [#1354] and am unaware of this change. Thanks.
Comment #75
YesCT CreditAttribution: YesCT commented@Lars Toomre I think my statement in #73 was not clear. I meant to say that I was wrong in #65 and what I learned was that @ingroup is to be used instead of @params for the form and form state. http://drupal.org/node/1354#forms
It says:
@param and @return for the standard parameters and return value should be omitted from all form-generating functions. ... etc.
I'll work on this.
Comment #76
YesCT CreditAttribution: YesCT commentedI tried to look at the css style guidelines
http://drupal.org/node/302199 CSS coding standards
and recent css that got into core
#1238484: Ability to mark the form buttons to be of a specific type (so that they can be styled differently)
I dont think the standards are applied very strictly but there were a few things that seemed like most are doing.
I changed the hex to lower case,
added the /* LTR */
added some /* ---- Section ---- */ comments
I took out the blank lines that were between related blocks
js http://drupal.org/node/172169 JavaScript coding standards
Some coding standards and docs changes in the rest. I had a couple questions I'll follow-up with.
Comment #77
YesCT CreditAttribution: YesCT commentedI didn't see where there was a submit or validate for this form.
Looks like it's called from
core/modules/project_browser/project_browser.module: $build['install-link'] = drupal_get_form('project_browser_install_button_form');
I'm not sure if it needs a @see.
There are a few places with this pattern:
so is $list an array of arrays? I'm not sure what the [] does.
These seemed like they might need periods at the end of the sentences. But since I didn't do manual testing, I'm not sure where those phrases show in the ui.
Also, #72 still needs addressed.
Comment #78
YesCT CreditAttribution: YesCT commentedI found a few more things so that this can pass the doc core gate. I'm not trying to be difficult. :) Just really want this to get a good shot of getting in! http://drupal.org/core-gates#documentation-block-requirements
Needs period at the end of the sentence.
Needs an @see project_browser_test_menu()
Needs a better one line summary of the function and a typed @return
Needs a better one line summary, typed @param and typed @return.
This needs a better one line summary and a typed @return.
Comment #79
Anonymous (not verified) CreditAttribution: Anonymous commentedSorry it took so long, but here is the latest patch with the changes suggested by YesCT. I also reverted to the old method of getting the files from drupal.org with the update module, and commented out the new method with a @todo to change it once #1846078: Create a helper function that will install a project from local cache is ready.
I am still not sure what is causing the tests to fail for these patches. I can't duplicate the fails on my dev server with the latest code. Here's hoping this one works.
Comment #80
Anonymous (not verified) CreditAttribution: Anonymous commentedAnd the interdiff since my Chrome won't let me upload multiple files to one post for some reason...
Comment #81
Anonymous (not verified) CreditAttribution: Anonymous commentedOops, I must have botched the patch in#79. Here's a reroll.
Comment #83
Anonymous (not verified) CreditAttribution: Anonymous commented#81: pb_in_core_21b.patch queued for re-testing.
Comment #85
Anonymous (not verified) CreditAttribution: Anonymous commentedThe fails are in the views module's test class. Is this a bug in Views or did I cause this with the patch?
Comment #86
Anonymous (not verified) CreditAttribution: Anonymous commented#81: pb_in_core_21b.patch queued for re-testing.
Comment #87
Anonymous (not verified) CreditAttribution: Anonymous commentedYay, after a couple retests, it passed! Guess the test bot was having some troubles yesterday.
Comment #88
Bojhan CreditAttribution: Bojhan commentedCould you put up some latest screens?
Comment #89
Anonymous (not verified) CreditAttribution: Anonymous commentedHere are some screenshots. You can see the site yourself here:
http://d8.leightonwhiting.com/admin/modules
username: admin
password: test
Comment #90
Bojhan CreditAttribution: Bojhan commentedJust an initial review - a lot seems to be related to issues that simply haven't been worked on yet. Look at http://www.bojhan.nl/module-theme-browser-interface, for how I initially thought it should look - some details are off.
On another note, can we filter this list by version? E.g. when I am on Drupal 8, I really don't need Views, and can we also exclude Drupal Core, sounds highly unlikely you want to install that :P.
Themes seems broken in the demo.
Comment #90.0
Bojhan CreditAttribution: Bojhan commentedAdded some background info
Comment #90.1
Anonymous (not verified) CreditAttribution: Anonymous commentedUpdated the related issues
Comment #90.2
Bojhan CreditAttribution: Bojhan commentedUpdated issue summary.
Comment #91
Anonymous (not verified) CreditAttribution: Anonymous commented@Bojhan - Thanks for the review :)
Lots of those issues are caused by the incomplete Project Browser server implementation because it is running on D7 dev server, instead of the real Drupal site. The missing images, Drupal core and themes not working are because of this.
It is already filtering by version automatically. It will only show 8.x projects, since that's all that you can install on Drupal 8 anyways. The Drupal 7 version will show 7.x projects. I assume that the Views project must have an 8.x branch, which should probably be removed.
I'll move the sort by to the right. Checkboxes for the install queue seems a bit odd to me. Should they just always be checked, and if they are unchecked, they get removed?
Comment #92
Bojhan CreditAttribution: Bojhan commented@wildkatana Most of my concerns are with the styling that is off, we can spend some time on IRC going through them. I guess the reason that checkbox is wierd, because initially I imagined you could either "install" directly or "install from a queue" where that queue would remain the same. You merged those concepts, which is fine - I'd have to think what the most logical process is for adding/removing things from that queue - but using the error icon, mostly definitely is not.
Comment #93
Anonymous (not verified) CreditAttribution: Anonymous commentedI got rid of the images and just used core images and jquery theme. I moved the sort to the filters box, and moved the results count to the right side. I also added primary buttons where applicable.
I still don't think that checkboxes would be the best for the install queue, because if we use form elements, we should use a form submit button as well, such as 'remove from install queue', and that would get confusing I think. Also, if we use links like it is now, it works when JS is disabled.
What do you think?
Comment #94
Bojhan CreditAttribution: Bojhan commentedThis really needs some markup review, I noticed loads of things going through the code that have special styling, or no styling - I am no front-ender, I just noticed these doing a visual review to write a quick list:
Just an idea, perhaps we can place last updated on the same line as author. Makes it a tad bit nicer on the eye :) I dont know yet about the whole removing and adding to the queue, currently - I will think some more about it.
Comment #95
Bojhan CreditAttribution: Bojhan commentedComment #96
Anonymous (not verified) CreditAttribution: Anonymous commented@Bojhan - Thanks for the review. Here are my responses:
Which tables are you referring to? There are H2's used for the block headers, is that what you mean? Would a table header really make sense if a table isn't being used?
How should I go about doing this?
Is there a link somewhere with core colors that can/should be used? I'll go through and see if I can remove some.
I'll review these and remove where possible.
By this you mean it should scale when resized? What is the best practices to use here? Currently they are using percentages of the browser window, but it doesn't work well for very small window sizes.
I'll remove it.
I followed that link's instructions to make them. I think the problem is that when a project is added/removed from the queue and it is rebuilt with AJAX, it loses it's styling. Perhaps this is a bug with the primary button feature?
Comment #97
Bojhan CreditAttribution: Bojhan commentedAssigning to @sun, for more markup review - I wouldn't know how to answer most of these questions.
Comment #98
attiks CreditAttribution: attiks commentedI had a quick look at the demo, nice
I queued "IP-based Determination of a Visitor's Country" and in the Install queue block it shows as
IP-based Determination of a Visitor's Country
The image for HTML mail is wrong
<img src="http://drupal.org/node/1124934">
Comment #99
eigentor CreditAttribution: eigentor commentedI guess we should do a screen-by-screen UX Review with detailed comments and proposals how to improve the workflow.
If this gets only 75% right, it will help new users tremendously to install a module (and totally breaking their site with it :P)
Comment #100
Bojhan CreditAttribution: Bojhan commented@eigentor I reviewed the most important screen, all the others are just batch API. With many some text fixes required.
Comment #101
eigentor CreditAttribution: eigentor commentedWell I guess I will do a review as well. Screenshots help, though, maybe you can make some? Else it remains abstract.
Comment #102
Bojhan CreditAttribution: Bojhan commented@eigentor Please read the comments, there is a demo, and there are screenshots.
Comment #103
realityloop#81: pb_in_core_21b.patch queued for re-testing.
Comment #104
tim.plunkettI ran out of steam at project_browser_get_categories(). But a lot of this feedback can be applied to the patch as a whole.
----
missing @var docs
No docblock needed for setUp()
Should be protected
No need to wrap this
No need to wrap this
This is deprecated, soon to be removed, use details. See http://drupal.org/node/1852020
This should not be wrapped
I couldn't find any other core modules with a $module.inc, they all have $module.$foo.inc
No hyphen after @todo
Missing t()
What's with the $_SESSION?
Should use #type => link
Should return a render array. Also the indentation is off
This kinda just floats here, maybe a comment?
Returning NULL (or nothing, technically) would probably work just as well
Make this a ternary
Never use AND, always &&
Comment #105
Dries CreditAttribution: Dries commentedWhile I think this could be a game changer for Drupal, I don't actually think it is 'critical'. Something is critical when we wouldn't release Drupal 8 without this; we would realize Drupal 8 without this feature. Having said that, I'd still like to see this land as it has been a very serious usability issue for many releases. So I strongly encourage people to keep working on this!
Comment #107
eigentor CreditAttribution: eigentor commentedI wanted to give the Project Browser an UX Review, but I have trouble to get it up and running.
It is the same problem with installing the D7 version or trying the D8 demo site http://d8.leightonwhiting.com/admin/modules
Project browser does not appear to be able to fetch any search results from d.o. anymore http://screencast.com/t/RJtrCFlY. As I understand, you are working on deploying the server module to d.o. to fix this permanently, and up to now it was run from a d.o. dev site.
But it does not appear to work anymore.
Comment #108
douglasmiller CreditAttribution: douglasmiller commentedThis module's config/project_browser.settings.yml file sets the default search server to be http://pbs-drupal_7.redesign.devdrupal.org/project_browser/server (drupal:drupal). Unfortunately, that devdrupal.org site is returning a 404 when requesting that page.
The issue summary mentions that this module requires that drupal.org be running the project_browser_server module: #1243332: Deploy Project Browser Server and drupalorg_pbs on d.o. I assume that the devdrupal.org above WAS running that module, but that no longer appears to be the case.
I've requested a new devdrupal.org site and will install the server module there if my request is granted. If I can get it working, I will roll a new patch for this issue with an updated settings.yml file.
Comment #109
YesCT CreditAttribution: YesCT commentedI'd really like to help with this, but looks like we are waiting on the server?
Comment #110
YesCT CreditAttribution: YesCT commentedI'm going to do a re-roll (if needed) and some coding style stuff. I'll post back what I get.
@jthorson is helping a bit on server issues but @c4doug and I are not quite sure what is needed. (If I can speak for them)
Comment #111
YesCT CreditAttribution: YesCT commentedI could not find any examples in core of types on test user vars, or @var for them.
the (possibly out of date) standard for tests says setUp is public.
I'm not sure how to do the conversion to return a render array.
I'm not sure how to convert l() to '#type' = > 'link'. because I dont know what to do with the extras in the third argument to l().
I'm not sure if I should move the .inc file to some general $foo.inc, or to split up the pieces into many $whatevers.inc's.
And... I think I have messed up something (maybe the fieldset conversion to details) because the settings page has an error (and no settings).
but here it is.
Only went through 3 files. I'm happy to do some more, hopefully once have more info on how to handle those situations.
Comment #113
YesCT CreditAttribution: YesCT commentedHere I've pointed out the likely problem areas to make it easier to answer the questions.
this actually looked ok. but the settings page didn't work.
I need some help, or examples to look at, for these two.
I was not sure how to change this to 'return a render array'. Where is an example to look at?
Comment #114
chx CreditAttribution: chx commented> User vars
@var \Drupal\user\Plugin\Core\Entity\User
if it's a user object.> I need some help, or examples to look at, for these two.
#options => array('query' => $query, 'class' => array())),
> I was not sure how to change this to 'return a render array'. Where is an example to look at?
core/modules/user/lib/Drupal/user/Plugin/block/block/UserLoginBlock.php has a #theme item_list. node.module has
return $num_rows ? array('#theme' => 'item_list__node', '#items' => $items, '#title' => $title) : FALSE;
As for protected/public setup, I grepped http://privatepaste.com/38ddd30fc9 and there is no standard so don't sweat on it, it's off topic.
Comment #115
YesCT CreditAttribution: YesCT commentedstill guessing on the render array.
fixed the @var for user, and the #type => link to use the options, changed one of the #type => links to link
found a missing newline before the closing brace of a class
and fixed a couple syntax errors with mismatched parens and curly braces
Comment #116
YesCT CreditAttribution: YesCT commentedhelp standards:
http://drupal.org/node/632280
updated help to use the right format, but the content of the Uses section needs to be corrected. There are @todo's in that section for reminders to do that.
other standards stuff.
Next
This is it for my first pass. Others are welcome to address some of the issues that have been brought up in reviews since November, like $_SESSION and markup, admin/config/development/project_browser not working, etc.
Comment #117
YesCT CreditAttribution: YesCT commentedthese are the notices on the settings page:
Comment #118
eigentor CreditAttribution: eigentor commentedCan somebody apart from Wildkatana re-set-up the server? This really needs a demo to be tested. Maybe someone can ask him how this works.
about what is needed: The server makes sure that the module can receive a list of Drupal projects. This is so because a certain module http://drupal.org/node/1243332 is not yet deployed on d.o. so the module (or say better project) list cannot be directly pulled from d.o., but from that duplicate that is the demo site instead.
Comment #119
YesCT CreditAttribution: YesCT commented@eigentor
I think @c4doug and @wildkatana are on it: #1897088: I need a dev server to test the d8 project browser module
but are stuck at the moment.
Comment #120
vijaycs85Re-rolling for admin configuration page with some config file changes.
Comment #122
YesCT CreditAttribution: YesCT commented@vijaycs85 thanks! I wanted to check it out, but it does not apply. Could you make a new patch (git diff 8.x) from a recent git pull?
Comment #123
vijaycs85@YesCT, I'm on windows machine and it is very painful to get the PNG files proper in my patch. I have to remove PNGs to get the proper patch. Can we add it back, when it is really necessary (or as separate patch) as we can't do review them in patch?
Comment #125
dman CreditAttribution: dman commentedJust a drive-by patch review (part of drupalcon review sprint)
Patching and enabling went in clean for me against todays D8 head, but as soon as I hit /admin/modules/project-browser
This would imply that drupal_get_path_alias() has gone away.
That doesn't seem to be explicit in the changelog yet today.
(Sorry if this fast test review is off-topic, it looked like a fun D8 improvement to demo to the learners here.)
Comment #126
tim.plunkettIt was included in the change record http://drupal.org/node/1853148, which was the first result.
Comment #127
dman CreditAttribution: dman commentedAh yes. Good-O!
I guess mentally filtered out an old change from Nov 2012, not expecting it to be a API change that would now be suddenly affecting a new patch rolled in Feb 8.
Comment #127.0
dman CreditAttribution: dman commentedUpdated issues again
Comment #128
YesCT CreditAttribution: YesCT commentedvijaycs85, yeah, leave out the images and do what you have to do to make a patch that applies. it might confuse the interdiffs, but we need all hands on this issue, so happy to have your set of eyes on this.
... as long as you dont include the images in the diff you do to make the interdiff, the interdiff should be spot on, even if the patch does not have the images...
is a virtual machine an option? maybe vagrant? http://drupal.org/project/vagrant
Lets see if we can get other devs using windows to chime in here.
Comment #129
sunI'm sorry — to clarify a couple of things:
1) I only briefly reviewed the project browser server/client originally, back around BADCamp 2011. That was mainly an architectural but certainly not an in-depth review.
2) Important: A few earlier comments in here or in the related server issue referred to my reviews; in particular with regard to security review questions. A security review of the client/server code never happened (at least from my side).
3) I won't be able to review and spend time on this + the sister issue(s) anytime soon, so in order to manage expectations, I'm unassigning myself. I'm still following this issue, but I hope that someone else can pick this up to provide architectural + technical reviews and directions.
Comment #130
Anonymous (not verified) CreditAttribution: Anonymous commentedSorry for the long silence on my part, been a hectic month with the new kid. I am going to be concentrating on getting the dev site which is running Project Browser Server and drupalorg_pbs working so that testing can continue. If anyone needs anything from me, please PM me. And thanks for all the patch reviews and help on this!
Comment #131
YesCT CreditAttribution: YesCT commented@sun thanks!
@wildkatana thanks too, I know you are being pulled in two directions.
Once we get this up and running, reviews (and a security review) will be much easier to do.
Comment #132
YesCT CreditAttribution: YesCT commentedwe are still in need of some work to get this running so we can work on it. (#1897088: I need a dev server to test the d8 project browser module)
Comment #133
douglasmiller CreditAttribution: douglasmiller commentedI have enabled an example module on the server side that will serve a VERY limited data set. There are 3 modules and 3 themes. It isn't much, but at least this module is functional again.
Here is the data that can be returned for others to use as a reference when testing:
Modules
Themes
Comment #134
Bojhan CreditAttribution: Bojhan commentedAs sad I am about doing this, the progress on this issue has been incredibly sporadic focused around deadlines - the contrib module is still not very functional and we have to depend highly on the d.o team - which for the next few months will still be very busy. With those reasons in mind I am moving this to Drupal 9, it was not near RTBC on the day of freeze and it is not a critical piece of functionality for releasing D8.
:(
Comment #135
Anonymous (not verified) CreditAttribution: Anonymous commentedSad to see this as well :(
In the end I think the holdup was getting #1243332: Deploy Project Browser Server and drupalorg_pbs on d.o done.
I think the best way to go for now is getting the contrib module working, which still requires drupal.org to run Project Browser Server. Once we have that going and a working contrib that people can use and improve on, we have a better shot at Drupal 9.
Comment #136
YesCT CreditAttribution: YesCT commentedComment #137
dwwUpdate status spent a full core cycle (D5) before we put it in core in D6. That definitely helped flesh out some problems that were easier to fix in contrib than they would have been had it already been in core. So, as nice as it would have been to have this in D8, it's not the end of the world if it spends some time maturing in contrib before it's added.
That said, I'm sorry that deploying the server on d.o was a major hold-up. d.o is a hard site to work on for all sorts of reasons I don't want to enumerate here...
Comment #138
webchickThis stuff was originally ready to go in summer of 2011. That would've been more than enough time to let this mature in contrib before proposing it for core in Drupal 8, even by the original feature freeze date. It's both incredibly sad and immensely frustrating that our infrastructure workflow is so gummed up that it's now actively causing harm to the Drupal product itself, which it's intending to support.
Comment #139
cweagans+1 for adding an exemption for this feature. It was ready long before feature freeze and was only blocked on infrastructure changes. In the issue opened to deploy project browser server on Drupal.org, I've volunteered to spend time on whatever needs done.
Comment #140
YesCT CreditAttribution: YesCT commentedI would not call it ready to go, but It was ready to begin the core polishing process in terms of coding standards and accessibility and consistant ui with core. we tried to start that in ... #104 December 28 with @tim.plunkett's review. But could not really get rolling without both pieces in place.
Comment #141
sunSince this mainly affects the client-side UI only (as most of the API-level plumbing exists since D7 already), I cannot see why a well-tested and sophisticated implementation can't happen at any time; be it 8.x-dev, 8.0 or 8.1 or 8.[whatever].
The only procedure I'd recommend is this:
hidden = TRUE
in its .info file.Therefore, I'm moving this back to 8.x. Call it an "exception" or "whatnot", I don't really care. Decisions like this should be based on API-level changes only.
And if there happens to be a printed book that documents the process of discovering, downloading and installing a Drupal module, then I couldn't care less — we inanely rejected improvements to Drupal core on that basis in the past already, but a printed book is the utterly wrong medium for documenting and explaining a web-based application in the first place. Let's not hinder progress, just because of a stone-age medium that is not able to cope with the dynamics of the topic it tries to document, kthxbye.
I'd like to encourage everyone to move forward here. This is a very complex topic, which will need its time to get done. And, it requires the combined knowledge and expertise of all Drupal core contributors to get it right. Shifting it off into contrib will not help in any way; in fact, it would only be guaranteed to become worse, UX-wise and code-wise.
Lastly, speaking of overall maturity, I think it makes sense to manage expectations for everyone involved: This did not land before any feature freeze deadline and most probably won't land anytime soon, so it has to be 100% picture-perfect when it will land, since follow-up issues will not be possible.
Therefore, I'd like to shift the thinking to rather expect that we'll likely need and see a "Add project browser #2" issue after reaching 300 comments on this issue here, to get the last 20% done. As we all know, the 80% of getting it to work is the simple part of the game only. :) Especially in this case. That's definitely not meant to discourage anyone from contributing here — but instead, the full-frontal opposite: Aim for the picture-perfect, and don't ignore any details. Give a true meaning to "battle-tested", so that we can be sure that both the UX as well as the code meets each and everyone's expectations.
This is doable. Let's do it.
Comment #142
David_Rothstein CreditAttribution: David_Rothstein commentedI think it may be premature to move this issue to Drupal 9, for a couple of reasons:
I'm not necessarily saying a new module should be added to Drupal 7 at this point. (Although such things have been discussed - see http://groups.drupal.org/node/210973#comment-700093 - and I don't have an objection to it myself if it's been heavily tested.)
But I am saying that given the above, surely it can still be on the table for Drupal 8? :) The current patch makes zero changes anywhere else in core. (In practice I think Update Manager might need to change a bit to work smoothly with it, but those kinds of refactorings are definitely not off the table for Drupal 8 yet.)
We wouldn't want to do this for any random module, but in my opinion this one is a unique case.
#1810428-111: [Policy, no patch] Adjust major and critical task thresholds during code thaw
#1410544: Consider possible ways to shorten code freeze [policy, no patch]
Based on those comments, it seems quite likely that some features will still be acceptable in Drupal 8 even past the official feature freeze date. There are definitely open questions about how big of a feature or what the criteria would be, as well as how this would tie into the mess that "thresholds" have become... but I get the impression there's at least a theoretical chance.
Comment #143
David_Rothstein CreditAttribution: David_Rothstein commentedCross-posted with @sun :)
Comment #144
David_Rothstein CreditAttribution: David_Rothstein commentedAlso, looking at some of the code, if there's a need to do manual testing of this module (e.g. to refine the user experience) while things on the drupal.org end aren't working correctly, can't the test module which is already included in the patch be used for that?
For example:
hidden = TRUE
line (could even be done in the patch itself, temporarily).I believe at that point, you should just be able to turn the "Project Browser module tests" module on via the user interface on your local site and be able to use a limited version of it (with only certain modules and themes appearing, of course) without ever having your site contact drupal.org?
I have not tested to see if that's true (the current patch doesn't work with the latest D8 code so I can't). But if it's not entirely true, then that's just an incentive to add code to the test module to make it as true as possible - which would have the nice side effect that it improves the automated test coverage at the same time :)
Comment #145
falcon03 CreditAttribution: falcon03 commented@YesCT#140 (and anyone else): Most of the accessibility issues have already been fixed in november 2012.
Of course, +1 to get this into D8! :-)
I'm not removing the "needs accessibility review" tag just to do a final review of the patch when it is ready to be committed.
Comment #146
douglasmiller CreditAttribution: douglasmiller commentedIt looks like there is still a strong interest in this, so I got to work rerolling the patch.
I fixed a couple of other bugs, but there is another that I am stuck on. The /admin/modules/project-browser/modules page outputs Array below the pager (showing x to y of z). There should be a item list of sort options instead of Array.
The interdiff may not be 100% accurate. I didn't apply the .patch from #120. I used the interdiff instead.
Comment #147
Anonymous (not verified) CreditAttribution: Anonymous commented@David_Rothstein - There is an example implementation of the Project Browser Server API that can be set up to run on the staging site which can return static results, until the Solr is fixed for real results. It won't have more than a few projects but we can at least use that for testing and refining this while we get Solr working at the same time.
@c4doug & YesCT - Thanks for the patches and effort here, I've been super busy the past couple months but I am getting more time next week to push on this :D
@Everyone - Yay, thanks for all the support, I really truly feel that this will make an awesome Core usability improvement. I can't tell you how many clients of mine don't know how to install a Drupal module, so lowering the barrier will definitely help the community here!
And yes, this can definitely run on D7 as well, in fact I made it for D7 in the first place. Having a D7 contrib is in the plans.
Comment #148
catchThere's a bunch of major bugs with update module, and they get very little attention:
http://drupal.org/project/issues/search/drupal?status%5B%5D=Open&priorit...
Comment #149
japerryThis is fairly out of date, working on updating it to the new routing system and other yml files.
Comment #150
japerryFor those interested in this issue, work in continuing at PNWDS in this sandbox. PM me if you'd like contributor access.
https://drupal.org/sandbox/japerry/2105593
Comment #150.0
japerryIssue summary added based on template
Comment #151
Leeteq CreditAttribution: Leeteq commentedUpdated, link in #148 not working.
Current list of open D7+D8 major+critical bugs related update.module:
https://drupal.org/project/issues/search/drupal?text=&assigned=&submitte...
Comment #152
RainbowArrayJust curious, what happened to this issue? It looked like there was some good progress being made in February 2013, then the conversation just stopped. It looks like a Sandbox was set up, but it doesn't seem as if there has been activity there until October 2013.
I can imagine it would be difficult to get this into d8 at this point (although it would be super useful). At the very least it would be nice to have this in contrib and get it to the quality where it could maybe be pulled into core in a feature release of d8.
The problem this addresses has always seemed like a big gap as far as I can remember. Would love to see the awesome work done here come to fruition.
Comment #153
RainbowArrayAccidental dup.
Comment #154
webchickI think the code freeze around porting d.o to Drupal 7 happened, which then lasted an extra ~year than expected. :\ I agree it'd be great to see someone pick this up again.
Comment #155
aneek CreditAttribution: aneek commented@webchick, do you have any recent status on this? I think its a good feature to have. However, I can take on this issue and make it work for D8.
@japerry, I checked out the latest from https://drupal.org/sandbox/japerry/2105593 sandbox and it seems that for quite a few months it has stopped development. 1st thing needed is to convert PSR-0 to PSR-4 conversion. If its fine with you then please grant me git contributor access.
Comment #156
webchickI know of no recent progress on this issue, nor on the d.o dependency at #1243332: Deploy Project Browser Server and drupalorg_pbs on d.o. :( If you want to help, probably getting a d.o sandbox set up over there would be the first step. drumm, tvn, or basic in #drupal-infrastructure on IRC could probably point you in the right direction. Would be SO great if we could do this, if not for 8.0.x then at least for a later minor release.
Comment #157
webchickComment #158
aneek CreditAttribution: aneek commentedThanks @webchick for the information. I've tried to connect to the contributers for this feature. Lets see what happens. Btw, I'd like to add a new feature to this.
If this one is moved to core, then I'm sure many users will use this thats for sure. But a lot of site maintainers have different approach to place the modules or themes to their site's directory.
Say for example, in D7 based sites, its very common that many of us have directory called "contrib" & "custom" in /sites/all/modules/. This same may happen while users will use D8 as well. Some may also place their modules in /profiles directory for their own customized distributions.
This idea I am suggesting is, to give the user an option to place the modules or themes to their desired locations. As of now there we don't have any automated way to place the modules/themes except using Drush (--destination="").
While the user selects any module or theme, just before installation ask for a custom path or may be a global settings in admin/config to enable custom paths for modules and themes.
Comment #159
drummInstructions for getting a Drupal.org dev site set up are always at https://www.drupal.org/node/1018084.
#1243332: Deploy Project Browser Server and drupalorg_pbs on d.o isn't necessarily the best direction forward for Drupal.org at this point. If we are doing something highly specialized for Drupal core's project browser on Drupal.org, I want to be sure the core side is on track to be committed. If we are taking on the maintenance effort for a special API, it needs to be used.
However, we've generally been working hard to reduce project*-specific magic and use more modern Drupal architecture. A more-general API for Drupal.org projects would be useful for more than this issue.
I think the best thing to do today would be to take a step back to look at what's needed out of Drupal.org's APIs and the best way to implement that today. For example, RestWS is now installed on Drupal.org and has querying capabilities, how far can that get us? It does query via EntityFieldQuery instead of Solr, so it might not work out of the box. It is sufficiently overrideable/extendable to work well, with less code in the end? Are there other ways to go with less hardcoding to Drupal.org's configuration than http://cgit.drupalcode.org/drupalorg_pbs/tree/drupalorg_pbs.module?h=7.x... has?
Comment #160
webchickWere this feature to be ready, it would absolutely be committed to core. This is a huge gaping hole compared to competitors like WordPress, and we've seen in usability study over usability study that when people are taken out of their Drupal install and forced over to Drupal.org to find things, suddenly their experience takes a dramatic turn for the worse. The only question is whether it would be 8.0.x or 8.1.x or whatever, and to me that depends on timing of when this is ready and what phase of the release cycle the 8.x branch is in at that point.
Comment #161
steinmb CreditAttribution: steinmb commentedUn-assigning @japerry. Pls. re-assign if you are active working on this issue.
Comment #162
jhedstromFeature request => 8.1.x.
Comment #163
japerryI'll reassign when the d.o infra can support this feature. Right now there is a list of issues keeping this from happening, but an 8.1 or 8.2 release goal is realistic.
Comment #166
sarmiliboyz CreditAttribution: sarmiliboyz commentedIs there any update on this?
It would be great if we can browse module/themes inside our admin area. With this feature we can stand with wordpress and backdrop from newcomer perspective.
Comment #167
sarmiliboyz CreditAttribution: sarmiliboyz commentedCan we include this to drupal core ideas so we can make this solid?
Comment #168
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedWhat specifically do you want to achieve by moving it there?
This issue has already been open for years, has had UX reviews and conceptual reviews in the comments above, etc. Everyone seems to be on board with it as an actual idea, as far as I can tell. Plus there are a lot of patches that have been written. I think what needs to be done is to finish and polish the actual implementation, and then get it committed.
Maybe creating a separate issue in the ideas queue to develop parts of the idea further is a good idea, but I think moving this one there will tend to hide all the existing work that's been done on it. (And it's now been in the ideas queue for two and a half months and no one has commented on it since then at all...)
Comment #169
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedComment #172
andypostLooks this one still should be moved to ideas
Comment #173
catchYes agreed, moving it there.
Comment #174
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedSee my comment in #168 - what is the benefit/goal of moving this to the ideas queue? What specific idea needs to be further developed?
Also, as @drumm noted above, it is really worth thinking about whether the new Drupal.org API (https://www.drupal.org/drupalorg/docs/api) can simplify the drupal.org requirements for this issue, and thereby allow the Drupal core patch to proceed with fewer blockers. Being able to use Solr for the searches would be ideal, but perhaps the existing API plus #2392183: Support for contains or starts with operators for filter parameters added to drupal.org (to allow very basic full-text searching via RestWS) would be enough for a first pass?
Comment #175
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedComment #176
webchickYeah, not sure we need this in Ideas queue. It's pretty much a no-brainer to do.
Here's what WordPress's version looks like, which means this is what 27.5% of the Internet expects out of the experience of extending their CMS:
Comment #177
drummWe actually do have a basic project search with Solr API now, thanks to Composer: https://packages.drupal.org/8/search.json?s=carousel is the machine-readable version of https://www.drupal.org/project/project_module?f%5B0%5D=&f%5B1%5D=&f%5B2%...
The only available parameters are the search string
s
and paginationpage
. Adding more is probably okay, as long as it doesn't affect what the Composer CLI requests expect.Comment #178
gagarine CreditAttribution: gagarine as a volunteer commentedUsability is preferred over UX, D7UX, D8UX etc. See https://www.drupal.org/issue-tags/topic
Comment #179
kevinquillen CreditAttribution: kevinquillen at Velir commentedI think this is a great idea.
It may have been mentioned before but I don't see it (did a search for composer) but this can only be a reality if the downloading mechanism is Composer (for D8+) based or prevents installation of modules that have Composer based requirements (such as needing external libraries, like Solarium). Without that, it will crash the site or prevent desired functionality for those users, like this.
edit: I say this assuming users would be able to 'download' from this UI.
Comment #180
David_Rothstein CreditAttribution: David_Rothstein commentedMoving this back to the core queue, based on the above discussion. Also there's #2940733: Site Builder Tool/Project Browser initiative which was created in the interim, and that one is in the Ideas queue.
That's pretty cool! It really suggests that an initial version of this feature could perhaps work very well without any drupal.org changes at all.
Comment #181
David_Rothstein CreditAttribution: David_Rothstein commentedI don't think so - this issue is mainly about downloading projects. Preventing installation of modules that have (unmet) Composer requirements is definitely a good idea, but it would certainly be important for the normal way to install modules also (e.g. from the Extend page), not specific to this issue.
Currently I think it's still up to the individual modules to do this in hook_requirements() (as it always has been), but there's an issue at #2494073: Prevent modules which have unmet Composer dependencies from being installed which proposes making core do it automatically.
Comment #182
David_Rothstein CreditAttribution: David_Rothstein commentedBack to "needs work" also, since there's still a patch here (but it's old).
Comment #184
AaronMcHaleThis is definitely something that is needed in Drupal, after scanning through the discussion I have a couple of thoughts:
The first one is based on screenshots I've seen where the project browser has "modules" and "themes" tabs, this is ok, but I think it would be a better experience if the modules and themes search/install pages were located under the existing Extend and Appearance pages, like how both of those pages have their own "Install new module" and "Install new theme" buttons that take you to an install page. We should take over those buttons and install pages, so renaming those buttons to "Install and find new modules" and "Install and find new themes", by clicking those buttons the user would see the search pages and then have the additional options tucked away on those pages for installing a project manually.
The second thought is that we need to be Composer aware, in other words we need to find a way of keeping a composer.json file up to date with the module and themes that are being installed, along with resolving any of their dependencies. However we also need to keep in mind that we shouldn't be modifying the composer.json provided by the Drupal Core project, we should only do it if one exists outside of the site/doc root, such as when using the drupal-composer project or similar. Having said that we may want to hold of on Composer integration until Phase 1 of #2958021: Proposal: Composer Support in Core initiative is implemented since that will ensure a standard structure for how Composer will work in Drupal going forward. In the meantime we should at least be aware of #2494073: Prevent modules which have unmet Composer dependencies from being installed and how that might impact the installation process using the project browser.
Comment #185
AaronMcHaleJust a little restructuring as #2940733: Site Builder Tool/Project Browser initiative seems to make more sense as the parent issue since it covers this and possibly more as an initiative.
Comment #186
AaronMcHaleMy comment on #1243332: Deploy Project Browser Server and drupalorg_pbs on d.o:
If that is the case and the Project Browser can just query packages.drupal.org as the server that will simplify things a bit here.
Comment #187
sinasalek CreditAttribution: sinasalek at Practicalidea commentedI think as Drupal community we really love to make things over complicated. Does wordpress plugin browser support composer, is it even secure? or does it do any real dependency check? No, not even close. Honestly this sort of tools are for end users, we(developers) are pretty much comfortable using cli and composer. I think https://www.drupal.org/project/project_browser does a great job (yes it has some issues but nothing that can't be fixed) and it's already there, i was surprised that it hasn't been ported to Drupal8. probably because of all the discussions about a full fledged site builder with composer support which we all know is quite complicated to implement. I wish we could be more progressive, start small and quick and build on that. Drupal could have a decent UI project browser a long time ago.
Comment #188
AaronMcHale@sinasalek the issue would be though that there are many modules (a growing number) that require Composer since they have dependencies which are installed using Composer. So at the very least any Project Browser must be Composer-aware and either know how to handle modules that have Composer dependencies, or just not let you install those modules at all though the browser.
In addition there are a growing number of sites that (for obvious reasons) are using Composer to manage the e entire process of installing and upgrading Drupal core and modules. On top of that Composer use in the Drupal community will only continue to grow as the current efforts to get full Composer support for installing and upgrading into Core gains momentum.
In summary, there's no point in just rushing something out just to have something there, it's better to properly plan everything from the beginning, doing that in the long run you end up with a much better solution than you otherwise would have.
Comment #195
quietone CreditAttribution: quietone at PreviousNext commentedClosed #1355358: Allow searching for modules on Drupal.org from the modules page as a duplicate, adding credit