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.

CommentFileSizeAuthor
#176 wordpress-plugin-browser.jpg139.28 KBwebchick
#146 drupal-project_browser-1841788-146.patch104.39 KBc4doug
#146 interdiff-120-146.txt5.21 KBc4doug
#123 1841788-project-browser-123.patch102.44 KBvijaycs85
#120 1841788-project-browser-120.patch108.23 KBvijaycs85
#120 1841788-diff-116-120.txt5.11 KBvijaycs85
#116 drupal-project_browser-1841788-116.patch104.31 KBYesCT
#116 interdiff-115-116.txt18.48 KBYesCT
#115 drupal-project_browser-1841788-115.patch103.56 KBYesCT
#115 interdiff-111-115.txt3.24 KBYesCT
#111 drupal-project_browser-1841788-111.patch103.38 KBYesCT
#111 interdiff-28b-111.txt19.77 KBYesCT
#93 Capture.PNG63.08 KBwildkatana
#90 review-project-browser.png135.36 KBBojhan
#89 1_pb_modules_page.PNG49.85 KBwildkatana
#89 2_pb_listing_page.PNG74.18 KBwildkatana
#89 3_pb_select_versions.PNG23.17 KBwildkatana
#89 4_pb_install_dependencies.PNG23.06 KBwildkatana
#89 5_pb_enable_modules_page.PNG26.83 KBwildkatana
#81 pb_in_core_21b.patch106.21 KBwildkatana
#80 interdiff_20_21.txt36.53 KBwildkatana
#79 pb_in_core_21a.patch138.33 KBwildkatana
#76 drupal-project_browser_in_core-1841788-76.patch99.09 KBYesCT
#76 interdiff-65-76.txt19.8 KBYesCT
#65 drupal-project_browser_in_core-1841788-65.patch98.56 KBYesCT
#65 interdiff-62_20-65.txt2.75 KBYesCT
#63 diff.txt7.35 KBwildkatana
#62 pb_in_core_20.patch101.79 KBwildkatana
#59 pb_fixed_config.patch102.35 KBwildkatana
#55 interdiff.txt27.29 KBwildkatana
#53 project_browser_config_etc_1.patch104.14 KBwildkatana
#49 project_browser_accessibility_changes_2.patch102.71 KBwildkatana
#47 project_browser_accessibility_changes_1.patch102.78 KBwildkatana
#44 ss1.PNG112.46 KBwildkatana
#37 project_browser_in_core_take_6.patch118.17 KBwildkatana
#35 project_browser_in_core_take_5.patch118.74 KBwildkatana
#32 project_browser_in_core_take_4.patch124.77 KBwildkatana
#27 project_browser_in_core_take_3.patch118.02 KBwildkatana
#22 1841788-project_browser_in_core_whitespaces_fixes.patch112.81 KBandyceo
#21 1841788-project_browser_in_core_whitespaces_fixes.patch112.95 KBandyceo
#21 1841788-project_browser_in_core.interdiff.txt64.05 KBandyceo
#21 1841788-project_browser_in_core.interdiff_without_whitespaces.txt431 bytesandyceo
#18 project_browser_in_core_take_2.patch148.12 KBwildkatana
#9 multiselect.PNG31.48 KBwildkatana
#2 project_browser_in_core.patch111.86 KBwildkatana
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

webchick’s picture

Patch? :)

wildkatana’s picture

Status: Needs review » Patch (to be ported)
FileSize
111.86 KB

Here it is.

yoroy’s picture

Status: Patch (to be ported) » Needs review

RTBC when it comes back green ;-)
Exciting!

wildkatana’s picture

Status: Needs review » Reviewed & tested by the community

Looks like it passed to me.

sun’s picture

Priority: Major » Critical
Status: Reviewed & tested by the community » Needs review
sun’s picture

I 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.

yoroy’s picture

Yeah, that rtbc was a joke, this needs some more review first :-) Awesome that it turns up green though.

wildkatana’s picture

@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?

wildkatana’s picture

FileSize
31.48 KB

And the screenshot

yannickoo’s picture

@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?

yoroy’s picture

Before 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.

eigentor’s picture

I 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.

wildkatana’s picture

@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.

yannickoo’s picture

I just want to mention that the project name isn't in the url so that cannot work right?

nod_’s picture

Issue tags: +JavaScript

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.

webchick’s picture

Coding 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.

wildkatana’s picture

Status: Needs review » Needs work

@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.

wildkatana’s picture

Status: Needs work » Needs review
FileSize
148.12 KB

Okay, 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.

Bojhan’s picture

Assigned: wildkatana » Unassigned
Issue tags: +Usability, +d8ux

I would be great to get some more reviews on this, it will be a great addition.

aspilicious’s picture

I would love to get such a system in

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,880 @@
+  ¶
+  if ($cache = cache()->get($cid)) {
+    return $cache->data;
+  }
+  ¶
+  $results = array(
+    'projects' => array(),
+    'total' => 0,
+  );
+  ¶
+  unset($filters['server']);
+  ¶
+  foreach ($servers as $url => $server) {
+    $local_filters = $filters;
+    ¶
+    // We are not using this right now because we only expect to handle 1 server at a time currently
+    // $local_filters['requested'] = floor($filters['requested'] / count($servers));

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.

andyceo’s picture

andyceo’s picture

Sorry, miss to rework project_browser.pages.inc.

Lars Toomre’s picture

Thanks @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.

aspilicious’s picture

Status: Needs review » Needs work

I 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

wildkatana’s picture

@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...

Fabianx’s picture

#25: IIRC you need to declare libraries that you want to load now via #attached library property or so. That would explain the error.

wildkatana’s picture

Status: Needs review » Needs work
FileSize
118.02 KB

Thanks 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

wildkatana’s picture

Status: Needs work » Needs review

Forgot to change the issue status.

Lars Toomre’s picture

Status: Needs work » Needs review

Thanks @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.

Status: Needs review » Needs work

The last submitted patch, project_browser_in_core_take_3.patch, failed testing.

Fabianx’s picture

#27 probably needs a re-roll due to the format of item_list being changed:

http://drupal.org/node/1842756

wildkatana’s picture

Status: Needs work » Needs review
FileSize
124.77 KB

Re-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.

Lars Toomre’s picture

Thanks @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.

Status: Needs review » Needs work

The last submitted patch, project_browser_in_core_take_4.patch, failed testing.

wildkatana’s picture

Status: Needs work » Needs review
FileSize
118.74 KB

I'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.

Status: Needs review » Needs work

The last submitted patch, project_browser_in_core_take_5.patch, failed testing.

wildkatana’s picture

Status: Needs work » Needs review
FileSize
118.17 KB

Here goes another try, after fixing the last issue with the data element.

falcon03’s picture

Is 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)...

falcon03’s picture

Issue tags: -D8UX usability +Usability

fixing tags, sorry...

wildkatana’s picture

Yes, it is running the latest D8 code and the latest project_browser patch.

falcon03’s picture

Ok, maybe this is an accessibility & usability review, not a bare accessibility one. Here my suggestions are:

  1. Turn "install new module" into a primary tab;
  2. not show any result if the search form hasn't been submitted yet;
  3. Sorting the results should be handled through the search form;
  4. The results differentiation between modules and themes should be handled through the search form too;
  5. Each module name (the "result title") should be wrapped into an "h3" heading;
  6. the title placed before the results should be not different based on the filter criteria; in other words, we should rename it something similar to "search results" (it's very good to keep it wrapped into an "h2" heading);

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...

wildkatana’s picture

@falcon03 - Thanks for the review, here are my comments:

Turn "install new module" into a primary tab;

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".

not show any result if the search form hasn't been submitted yet;

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)

Sorting the results should be handled through the search form;

I can add a Sort By select box in the filters box to do this. Would that work?

The results differentiation between modules and themes should be handled through the search form too;

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.

Each module name (the "result title") should be wrapped into an "h3" heading;

Good point, done.

the title placed before the results should be not different based on the filter criteria; in other words, we should rename it something similar to "search results" (it's very good to keep it wrapped into an "h2" heading);

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.

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…

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.

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...

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.

Bojhan’s picture

Can you post screens etc, to review?

wildkatana’s picture

FileSize
112.46 KB

Attached 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'.

falcon03’s picture

@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!

wildkatana’s picture

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…

I 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?

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…

I agree with you here, having clearer titles makes sense. I'll see if I can get this worked in.

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.

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.

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.

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 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).

I have changed it now to remove the multiselect. Should make this patch more manageable and accessible.

wildkatana’s picture

I'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.

Status: Needs review » Needs work

The last submitted patch, project_browser_accessibility_changes_1.patch, failed testing.

wildkatana’s picture

Strange. I'll try another pull and commit it again.

wildkatana’s picture

Status: Needs work » Needs review

And changing the status

wildkatana’s picture

Lars Toomre’s picture

I really like this proposed module and hope to see it in D8 core. Here are some notes from reading through the patch:

+++ b/core/modules/project_browser/lib/Drupal/project_browser/Tests/ProjectBrowserTest.phpundefined
@@ -0,0 +1,107 @@
+ * @file
+ * Definition of Drupal\project_browser\Tests\ProjectBrowserTest.
+ */

The standard for the @file docblock recently was changed. Now 'Definition of' should be 'Contains'.

+++ b/core/modules/project_browser/lib/Drupal/project_browser/Tests/ProjectBrowserTest.phpundefined
@@ -0,0 +1,107 @@
+ *
+ * @todo - Create a test for the get releases HTML page.

Is there a follow-up issue opened for this @todo?

+++ b/core/modules/project_browser/lib/Drupal/project_browser/Tests/ProjectBrowserTest.phpundefined
@@ -0,0 +1,107 @@
+
+  protected $privilegedUser;

This member needs a docblock.

+++ b/core/modules/project_browser/lib/Drupal/project_browser/Tests/ProjectBrowserTest.phpundefined
@@ -0,0 +1,107 @@
+    variable_set('project_browser_default_server', array(
+      $server_url => array(
+        'name' => 'Test Server',
+        'method' => 'json',
+      ),

Variables are going away in D8 in favor of config and state sub-systems. Can we make the adjustment now?

+++ b/core/modules/project_browser/lib/Drupal/project_browser/Tests/ProjectBrowserTest.phpundefined
@@ -0,0 +1,107 @@
+  /**
+   * Tests for errors on the Projects page with an empty search string.

I am unsure of what errors are being checked for in this method. Is the docblock incorrect?

+++ b/core/modules/project_browser/lib/Drupal/project_browser/Tests/ProjectBrowserTest.phpundefined
@@ -0,0 +1,107 @@
+    $this->assertTrue(_project_browser_is_project_enabled('module', 'project_browser'),
+      t('Make sure project enabled detection works.'));

In last few months, all t() around messages in test assert messages have been removed. The same should be done here.

+++ b/core/modules/project_browser/project_browser.admin.incundefined
@@ -0,0 +1,36 @@
+
+
+/**
+ * Form constructor for the Admin Settings form.

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().

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,995 @@
+ */
+function project_browser_get_sort_widget($sort_options, $current_sort_option, $current_sort_direction) {
+  $sort_list = array();

Can we add 'array ' before $sort_options? Also elsewhere in this patch where an array is expected as function call variable type?

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,995 @@
+ *   (Optional) Set this to TRUE if you want to get all of the supported sort
+ *   methods. Defaults to FALSE.

Here (and elsewhere in patch) the explanation should start with '(optional)' ir lower-case.

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,995 @@
+ *   An associative array of queries to use to filter results, containing:
+ *   - 'version': The Major Version of Drupal that is running on the Client.
+ *     Example: '7' or '8'.
+ *   - 'text': The text that was entered as the search query, or '' if none.
+ *     Example: 'Link field'.

List documentation standard states that array keys like these should not be enclosed in quotes. Hence, '- version: '.

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,995 @@
+ *   Returns an array of results formatted like:
+ *   - 'total': The total number of results found for the filters.

Can just be 'An array of results'. Also same list formatting comment as before.

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,995 @@
+    // Cache this for 24 hours.
+    cache()->set($cid, $categories, strtotime("+24 hours"));

Perhaps this value of '+24 hours' should be set via config file?

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,995 @@
+  // @todo - Change the link once drupal.org is ready.
+  $servers = variable_get('project_browser_default_server', array(
+    'http://drupal:drupal@pbs-drupal_7.redesign.devdrupal.org/project_browser/server' => array(
+      'name' => 'Drupal.org',
+      'method' => 'json',
+    ),

Seeing this, this value should be set via a config file. It definitely is not a state() value.

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,995 @@
+ *
+ * @todo - Ideally, this should be in the update module as a standalone
+ * function, to reduce coupling and duplication.

Has there been an issue filed to add this function to update module?

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,995 @@
+ * @return array
+ *   An array indicating whether or not this was successful, and an error
+ *   message if applicable.
+ */
+function project_browser_download_project($url) {
+  module_load_include('inc', 'update', 'update.manager');

Missing '@throws \Exception' directive after blank line after @return directive.

+++ b/core/modules/project_browser/project_browser.moduleundefined
@@ -0,0 +1,529 @@
+    case 'module':
+      $title = t('Most used Drupal modules');
+      break;
+    case 'theme':
+      $title = t('Most used Drupal themes');
+      break;

Should be blank lines after each break;

+++ b/core/modules/project_browser/project_browser.pages.incundefined
@@ -0,0 +1,561 @@
+/**
+ * Form submission handler for project_browser_installation_enable_form()

Needs a period at end of line.

wildkatana’s picture

@Lars: Thanks for the review, I have corrected all of them and have the following notes:

The standard for the @file docblock recently was changed. Now 'Definition of' should be 'Contains'.

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 :)

Missing '@throws \Exception' directive after blank line after @return directive.

The exception isn't really thrown, it is actually caught in the method. Should I still document it even though it is handled?

Has there been an issue filed to add this function to update module?

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.

Lars Toomre’s picture

Thanks @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.

wildkatana’s picture

FileSize
27.29 KB

I'm not entirely sure I did this right, but here is the interdiff

YesCT’s picture

I 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"

Bojhan’s picture

I 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?

alexpott’s picture

Status: Needs review » Needs work

This is just a review based in the config file.

+++ b/core/modules/project_browser/config/project_browser.settings.ymlundefined
@@ -0,0 +1,6 @@
+cache_lifetime: "24 hours"
+# @todo - Change the link once drupal.org is ready.
+default_server:
+  "http://drupal:drupal@pbs-drupal_7.redesign.devdrupal.org/project_browser/server":
+    name: "Drupal.org"
+    method: "json"
\ No newline at end of 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 :)
  • the use of double quotes in yaml files is unnecessary if there is no spaces in the string
  • Needs a newline at the end of the file
wildkatana’s picture

FileSize
102.35 KB

@alexpott - Thanks, here is the full patch with those changes.

alexpott’s picture

+++ b/core/modules/project_browser/lib/Drupal/project_browser/Tests/ProjectBrowserTest.phpundefined
@@ -0,0 +1,126 @@
+  /**
+   * Tests a search for the Views string, ensuring that there are results.
+   */
+  public function testProjectBrowserSearchViews() {
+    // Create node to edit.
+    $edit = array();
+    $edit['search_text'] = 'views';
+    $this->drupalPost('admin/modules/project-browser/modules', $edit, t('Filter'));
+    $this->assertText('Showing 1 to');
+  }
+
+  /**
+   * Tests that the Projects Listing displays properly with the default filters.
+   */
+  public function testProjectBrowserGetProjects() {
+    // Attempt to fetch the default projects.
+    $edit = array();
+    $edit['search_text'] = '';
+    $this->drupalPost('admin/modules/project-browser/modules', $edit, t('Filter'));
+    $this->assertText('Showing 1 to');
+  }

Looks 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 the testProjectBrowserSearchViews to assert that it has found a views related project and has not found other not views related projects.

+++ b/core/modules/project_browser/project_browser.admin.incundefined
@@ -0,0 +1,35 @@
+  $form['main']['project_browser_servers'] = array(
+    '#type' => 'textarea',
+    '#title' => t('Repositories'),
+    '#default_value' => variable_get('project_browser_servers', ''),
+    '#description' => t("Add new repositories to use for the Project Browser, one per line, in
+      the 'url|method|Site Name' format. Drupal.org is added by default, and doesn't need to be
+      set here."),
+    '#required' => FALSE,

This needs to be getting the values from config.

+++ b/core/modules/project_browser/project_browser.admin.incundefined
@@ -0,0 +1,35 @@
+  return system_settings_form($form);

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.

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,992 @@
+    '#default_value' => isset($_SESSION['project_browser_text_filter_' . $type]) ? $_SESSION['project_browser_text_filter_' . $type] : '',

Don't think you should be using $_SESSION here... do you not have the value in $form_state?

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,992 @@
+      '#default_value' => isset($_SESSION['project_browser_category_filter_' . $type]) ? $_SESSION['project_browser_category_filter_' . $type] : NULL,

As above

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,992 @@
+/**
+ * Adds a project to the install queue $_SESSION variable.
+ *
+ * @param array $project
+ *   An array of $project data for a single project.
+ */
+function project_browser_install_queue_add(array $project) {
+  $_SESSION['project_browser_install_list'][$project['name']] = $project;
+}
+
+/**
+ * Removes a project from the install queue $_SESSION variable.
+ *
+ * @param string $project_name
+ *   The name of the project to remove, such as 'views'.
+ */
+function project_browser_install_queue_remove($project_name) {
+  if (isset($_SESSION['project_browser_install_list'][$project_name])) {
+    unset($_SESSION['project_browser_install_list'][$project_name]);
+  }
+}
+
+/**
+ * Gets the currently queued releases from the $_SESSION variable.
+ *
+ * @return array
+ *   An array of the currently selected releases.
+ */
+function project_browser_get_queued_releases() {
+  $releases = array();
+
+  if (isset($_SESSION['project_browser_install_releases_list'])) {
+    foreach ($_SESSION['project_browser_install_releases_list'] as $short_name => $info) {
+      if (is_array($info['project']) AND !empty($info['project'])) {
+        $releases[$short_name] = $info;
+      }
+    }
+  }
+
+  return $releases;

Why doesn't this use Drupal's queue api? See http://api.drupal.org/api/drupal/core%21includes%21common.inc/function/q...

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,992 @@
+    cache()->set($cid, $categories, strtotime("+24 hours"));

Shouldn't this be using the cache_lifetime config?

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,992 @@
+  if ($servers_raw = variable_get('project_browser_servers', '')) {

Should be using config()

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,992 @@
+  variable_set('maintenance_mode', $_SESSION['maintenance_mode']);

maintenance_mode has been converted to config()

+++ b/core/modules/project_browser/project_browser.infoundefined
@@ -0,0 +1,7 @@
\ No newline at end of file

Newline needed

+++ b/core/modules/project_browser/project_browser.pages.incundefined
@@ -0,0 +1,561 @@
+  $_SESSION['maintenance_mode'] = variable_get('maintenance_mode', FALSE);

Should be using config... I'm also skeptical that $_SESSION being used here.

+++ b/core/modules/project_browser/project_browser.pages.incundefined
@@ -0,0 +1,561 @@
+      // Add the release to a session variable.
+      $_SESSION['project_browser_install_releases_list'][$item['project']['name']] = array(
+        'release_name' => $item['release_name'],
+        'project' => $item['project'],

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!

alexpott’s picture

+++ b/core/modules/project_browser/config/project_browser.settings.ymlundefined
@@ -0,0 +1,6 @@
+    name: "Drupal.org"
+    method: "json"

Quick work - though you've still got unnecessary double quotes in the config file.

wildkatana’s picture

FileSize
101.79 KB

@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.

wildkatana’s picture

Status: Needs work » Needs review
FileSize
7.35 KB

And here is the diff.

Status: Needs review » Needs work

The last submitted patch, pb_in_core_20.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
2.75 KB
98.56 KB
+++ b/core/modules/project_browser/lib/Drupal/project_browser/Tests/ProjectBrowserTest.phpundefined
@@ -0,0 +1,111 @@
+}
+?>

end of class gets a newline.

And no ?> needed.

I'll fix that.

<code>
+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,913 @@
+function project_browser_install_button_form($form, &$form_state) {
...
+function project_browser_filters_form($form, &$form_state, $type) {

These 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.

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,913 @@
+ * Uses the update module to download and install a project.
...
+function project_browser_download_project($url) {

* Downloads and installs a project using the update module.

tells better what this function does.

+++ b/core/modules/project_browser/project_browser.infoundefined
@@ -0,0 +1,7 @@
+description = A Project Browser that allows users to browse for and install modules and themes from their Drupal site admin area

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"

+++ b/core/modules/project_browser/tests/project_browser_test.infoundefined
@@ -0,0 +1,5 @@
+name = "Project Browser module tests"
+description = "Support module for Project Browser related testing."

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

name = Views Test Config
description = Provides default views for tests.
package = Testing
version = VERSION
core = 8.x
dependencies[] = views
hidden = TRUE
+++ b/core/modules/project_browser/theme/project-browser-install.tpl.phpundefined
@@ -0,0 +1,22 @@
+

the other tpl.php files in this patch don't have this blank line.

+++ b/core/modules/project_browser/theme/project-browser-list.tpl.phpundefined
@@ -0,0 +1,27 @@
+
...
+

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

+++ b/core/modules/project_browser/theme/project-browser-project.tpl.phpundefined
@@ -0,0 +1,59 @@
+$class = ($first) ? 'project-item-first' : 'project-item';
+

does this need a @var?

+++ b/core/modules/project_browser/theme/project-browser-project.tpl.phpundefined
@@ -0,0 +1,59 @@
-- 

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.

YesCT’s picture

Status: Needs review » Needs work

sorry, should still be needs work for the test failures in #62

wildkatana’s picture

Status: Needs work » Needs review
Issue tags: -JavaScript, -Usability, -needs accessibility review, -d8ux

#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...

wildkatana’s picture

wildkatana’s picture

Hmm 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.

eiriksm’s picture

Just 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!

wildkatana’s picture

The 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.

YesCT’s picture

Provide 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.

YesCT’s picture

regarding 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.

Lars Toomre’s picture

@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.

YesCT’s picture

@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.

YesCT’s picture

I tried to look at the css style guidelines
http://drupal.org/node/302199 CSS coding standards
and recent css that got into core
#1238484: [docs update] 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.

YesCT’s picture

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,930 @@
+/**
+ * Form constructor for the install button for the Install Queue block.
+ *
+ * Since the selected projects are stored in the $_SESSION variable,
+ * no real processing is done, we just redirect to the install/select_versions
+ * page.
+ *
+ * @ingroup forms
+ */
+function project_browser_install_button_form($form, &$form_state) {
+  $form['#attributes']['id'] = 'project-browser-install-button-form';
+  $form['submit'] = array(
+    '#type' => 'submit',
+    '#value' => 'Install',
+  );
+
+  $form['#action'] = url('admin/modules/project-browser/install/select_versions');
+
+  return $form;
+}

I 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.

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,930 @@
+function project_browser_get_server_widget(array $servers, $current_server) {
+  $list = array();
+  $list[] = array(
+    'data' => t('Repository:'),
+    'class' => array('server-header'),
+  );

There are a few places with this pattern:

$list = array();
$list[] = array(

so is $list an array of arrays? I'm not sure what the [] does.

+++ b/core/modules/project_browser/project_browser.moduleundefined
@@ -0,0 +1,535 @@
+    'description' => 'Browse and search for new modules',
...
+    'description' => 'Browse and search for new modules',
...
+    'description' => 'Browse and search for new themes',

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.

YesCT’s picture

Status: Needs review » Needs work

I 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

+++ b/core/modules/project_browser/project_browser.pages.incundefined
@@ -0,0 +1,565 @@
+ * Form submission handler for project_browser_installation_enable_form()

Needs period at the end of the sentence.

+++ b/core/modules/project_browser/tests/project_browser_test.moduleundefined
@@ -0,0 +1,326 @@
+ * Page callback. Generates json based on the input filters.
+ */
+function project_browser_test_query() {

Needs an @see project_browser_test_menu()

+++ b/core/modules/project_browser/tests/project_browser_test.moduleundefined
@@ -0,0 +1,326 @@
+/**
+ * Returns some static categories.
+ */
+function project_browser_test_get_categories($type) {

Needs a better one line summary of the function and a typed @return

+++ b/core/modules/project_browser/tests/project_browser_test.moduleundefined
@@ -0,0 +1,326 @@
+/**
+ * Returns projects based on the filters.
+ */
+function project_browser_test_get_results($filters) {

Needs a better one line summary, typed @param and typed @return.

+++ b/core/modules/project_browser/tests/project_browser_test.moduleundefined
@@ -0,0 +1,326 @@
+/**
+ * Returns some static projects.
+ */
+function project_browser_test_projects() {

This needs a better one line summary and a typed @return.

wildkatana’s picture

Status: Needs work » Needs review
FileSize
138.33 KB

Sorry 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.

wildkatana’s picture

FileSize
36.53 KB

And the interdiff since my Chrome won't let me upload multiple files to one post for some reason...

wildkatana’s picture

FileSize
106.21 KB

Oops, I must have botched the patch in#79. Here's a reroll.

Status: Needs review » Needs work
Issue tags: -JavaScript, -Usability, -needs accessibility review, -d8ux

The last submitted patch, pb_in_core_21b.patch, failed testing.

wildkatana’s picture

Status: Needs work » Needs review

#81: pb_in_core_21b.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +JavaScript, +Usability, +needs accessibility review, +d8ux

The last submitted patch, pb_in_core_21b.patch, failed testing.

wildkatana’s picture

The fails are in the views module's test class. Is this a bug in Views or did I cause this with the patch?

wildkatana’s picture

Status: Needs work » Needs review

#81: pb_in_core_21b.patch queued for re-testing.

wildkatana’s picture

Yay, after a couple retests, it passed! Guess the test bot was having some troubles yesterday.

Bojhan’s picture

Could you put up some latest screens?

wildkatana’s picture

Here are some screenshots. You can see the site yourself here:

http://d8.leightonwhiting.com/admin/modules
username: admin
password: test

Bojhan’s picture

FileSize
135.36 KB

Just 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.

review-project-browser.png

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.

Bojhan’s picture

Issue summary: View changes

Added some background info

wildkatana’s picture

Issue summary: View changes

Updated the related issues

Bojhan’s picture

Issue summary: View changes

Updated issue summary.

wildkatana’s picture

@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?

Bojhan’s picture

@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.

wildkatana’s picture

FileSize
63.08 KB

I 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?

Bojhan’s picture

Issue tags: +markup

This 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:

  • Table headers, should not be specially styled H2's we can just have a normal table header?
  • There seems to be no RTL styling, this probally needs fixing.
  • There are some odd color definitions, not similar to elsewhere in core.
  • There are quite some font-sizes defined, most of them probably don't need special styling.
  • The search/category input forms don't seem to be responding very responsive, same for the items inside the table, and the two columns.
  • There is a strike-through effect, this is not part of core and should not be introduced here.
  • For primary button see http://drupal.org/node/1848288, it is now not correctly styled.

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.

Bojhan’s picture

Title: Project Browser in Core » Add project browser
wildkatana’s picture

@Bojhan - Thanks for the review. Here are my responses:

Table headers, should not be specially styled H2's we can just have a normal table header?

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?

There seems to be no RTL styling, this probally needs fixing.

How should I go about doing this?

There are some odd color definitions, not similar to elsewhere in core.

Is there a link somewhere with core colors that can/should be used? I'll go through and see if I can remove some.

There are quite some font-sizes defined, most of them probably don't need special styling.

I'll review these and remove where possible.

The search/category input forms don't seem to be responding very responsive, same for the items inside the table, and the two columns.

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.

There is a strike-through effect, this is not part of core and should not be introduced here.

I'll remove it.

For primary button see http://drupal.org/node/1848288, it is now not correctly styled.

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?

Bojhan’s picture

Assigned: Unassigned » sun

Assigning to @sun, for more markup review - I wouldn't know how to answer most of these questions.

attiks’s picture

I 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&#039;s Country

The image for HTML mail is wrong <img src="http://drupal.org/node/1124934">

eigentor’s picture

I 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)

Bojhan’s picture

@eigentor I reviewed the most important screen, all the others are just batch API. With many some text fixes required.

eigentor’s picture

Well I guess I will do a review as well. Screenshots help, though, maybe you can make some? Else it remains abstract.

Bojhan’s picture

@eigentor Please read the comments, there is a demo, and there are screenshots.

realityloop’s picture

#81: pb_in_core_21b.patch queued for re-testing.

tim.plunkett’s picture

Status: Needs review » Needs work

I ran out of steam at project_browser_get_categories(). But a lot of this feedback can be applied to the patch as a whole.

----

+++ b/core/modules/project_browser/lib/Drupal/project_browser/Tests/ProjectBrowserTest.phpundefined
@@ -0,0 +1,115 @@
+   * This is the user who will have access to use project browser.

missing @var docs

+++ b/core/modules/project_browser/lib/Drupal/project_browser/Tests/ProjectBrowserTest.phpundefined
@@ -0,0 +1,115 @@
+   * Overrides \Drupal\simpletest\WebTestBase::setUp().

No docblock needed for setUp()

+++ b/core/modules/project_browser/lib/Drupal/project_browser/Tests/ProjectBrowserTest.phpundefined
@@ -0,0 +1,115 @@
+  function setUp() {

Should be protected

+++ b/core/modules/project_browser/lib/Drupal/project_browser/Tests/ProjectBrowserTest.phpundefined
@@ -0,0 +1,115 @@
+    $this->assertTrue(_project_browser_is_project_enabled('module', 'project_browser'),
+      'Make sure project enabled detection works.');

No need to wrap this

+++ b/core/modules/project_browser/lib/Drupal/project_browser/Tests/ProjectBrowserTest.phpundefined
@@ -0,0 +1,115 @@
+    $this->drupalGet('project-browser/nojs/install-queue/add/views',
+      array('query' => array('destination' => 'admin/modules/project-browser')));
...
+    $this->drupalGet('project-browser/nojs/install-queue/remove/views',
+      array('query' => array('destination' => 'admin/modules/project-browser')));

No need to wrap this

+++ b/core/modules/project_browser/project_browser.admin.incundefined
@@ -0,0 +1,44 @@
+    '#type' => 'fieldset',
...
+    '#collapsible' => FALSE,
+    '#collapsed' => FALSE,

This is deprecated, soon to be removed, use details. See http://drupal.org/node/1852020

+++ b/core/modules/project_browser/project_browser.admin.incundefined
@@ -0,0 +1,44 @@
+    '#description' => t("Add new repositories to use for the Project Browser, one per line, in
+      the 'url|method|Site Name' format. Drupal.org is added by default, and doesn't need to be

This should not be wrapped

+++ b/core/modules/project_browser/project_browser.admin.incundefined
--- /dev/null
+++ b/core/modules/project_browser/project_browser.incundefined

I couldn't find any other core modules with a $module.inc, they all have $module.$foo.inc

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,1061 @@
+// @todo - Remove these once http://drupal.org/node/1846078 is committed.

No hyphen after @todo

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,1061 @@
+    '#value' => 'Install',

Missing t()

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,1061 @@
+    '#default_value' => isset($_SESSION['project_browser_text_filter_' . $type]) ? $_SESSION['project_browser_text_filter_' . $type] : '',
...
+      '#default_value' => isset($_SESSION['project_browser_category_filter_' . $type]) ? $_SESSION['project_browser_category_filter_' . $type] : NULL,

What's with the $_SESSION?

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,1061 @@
+      '#markup' => l($sort_option['name'], current_path(), array('query' => $query, 'class' => array())),
...
+      '#markup' => l($server['name'], $current_path, array('query' => $query, 'class' => array())),

Should use #type => link

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,1061 @@
+  return theme('item_list', array(
+      'items' => $sort_list,
+      'type' => 'ul',
+      'attributes' => array('class' => array('project-browser-sort-list'))
+    )
+  );
...
+  return theme('item_list', array(
+      'items' => $list,
+      'type' => 'ul',
+      'attributes' => array('class' => array('project-browser-servers-list'))
+    )

Should return a render array. Also the indentation is off

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,1061 @@
+  $i = 0;

This kinda just floats here, maybe a comment?

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,1061 @@
+ * @return array|false
+ *   Array containing all available categories or FALSE if no categories.
...
+  return FALSE;

Returning NULL (or nothing, technically) would probably work just as well

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,1061 @@
+  if (isset($_SESSION['project_browser_server_filter'])) {
+    $use_server = $_SESSION['project_browser_server_filter'];
+  }
+  else {
+    $use_server = 0;

Make this a ternary

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -0,0 +1,1061 @@
+  if (is_array($categories_raw) AND !empty($categories_raw)) {
...
+  if (is_array($categories) AND !empty($categories)) {

Never use AND, always &&

Dries’s picture

Priority: Critical » Major

While 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!

eigentor’s picture

I 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.

c4doug’s picture

This 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.

YesCT’s picture

I'd really like to help with this, but looks like we are waiting on the server?

YesCT’s picture

I'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)

YesCT’s picture

Status: Needs work » Needs review
FileSize
19.77 KB
103.38 KB

I 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.

Status: Needs review » Needs work

The last submitted patch, drupal-project_browser-1841788-111.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review

Here I've pointed out the likely problem areas to make it easier to answer the questions.

+++ b/core/modules/project_browser/project_browser.admin.incundefined
@@ -14,9 +14,8 @@
 function project_browser_admin_form($form, &$form_state) {
   $form['main'] = array(
-    '#type' => 'fieldset',
+    '#type' => 'details',
     '#title' => t('Main settings'),
-    '#collapsible' => FALSE,
     '#collapsed' => FALSE,
   );

this actually looked ok. but the settings page didn't work.

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -155,17 +154,20 @@ function project_browser_get_sort_widget(array $sort_options, $current_sort_opti
     $sort_list[] = array(
-      '#markup' => l($sort_option['name'], current_path(), array('query' => $query, 'class' => array())),
+      '#type' => 'links',
+      '#title' => $sort_option['name'],
+      '#href' => current_path(),
       '#wrapper_attributes' => array(
         'class' => $classes,
       ),
     );

@@ -209,7 +213,9 @@ function project_browser_get_server_widget(array $servers, $current_server) {
     $list[] = array(
-      '#markup' => l($server['name'], $current_path, array('query' => $query, 'class' => array())),
+      '#type' => 'link',
+      '#title' => $server['name'],
+      '#href' => $current_path,
       '#wrapper_attributes' => array(
         'class' => $classes,
       ),

I need some help, or examples to look at, for these two.

+++ b/core/modules/project_browser/project_browser.incundefined
@@ -155,17 +154,20 @@ function project_browser_get_sort_widget(array $sort_options, $current_sort_opti
+  // @todo Should return a render array.
   return theme('item_list', array(
-      'items' => $sort_list,
-      'type' => 'ul',
-      'attributes' => array('class' => array('project-browser-sort-list'))
+    'items' => $sort_list,
+    'type' => 'ul',
+    'attributes' => array('class' => array('project-browser-sort-list')),
     )
   );

@@ -218,10 +224,11 @@ function project_browser_get_server_widget(array $servers, $current_server) {
+  // @todo Should return a render array.
   return theme('item_list', array(
-      'items' => $list,
-      'type' => 'ul',
-      'attributes' => array('class' => array('project-browser-servers-list'))
+    'items' => $list,
+    'type' => 'ul',
+    'attributes' => array('class' => array('project-browser-servers-list')),
     )
   );

I was not sure how to change this to 'return a render array'. Where is an example to look at?

chx’s picture

> 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.

YesCT’s picture

still 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

YesCT’s picture

diff --git a/core/modules/project_browser/config/project_browser.settings.yml b/core/modules/project_browser/config/project_browser.settings.yml
...
+++ b/core/modules/project_browser/config/project_browser.settings.ymlundefined
...
+++ b/core/modules/project_browser/js/project_browser_select_releases.jsundefined
...
+++ b/core/modules/project_browser/project_browser.admin.incundefined
...
+ * Implements hook_help().

help 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.

YesCT’s picture

Status: Needs review » Needs work

these are the notices on the settings page:

    Notice: Undefined index: project_browser_admin in drupal_retrieve_form() (line 773 of core/includes/form.inc).
    Warning: call_user_func_array() expects parameter 1 to be a valid callback, function 'project_browser_admin' not found or invalid function name in drupal_retrieve_form() (line 817 of core/includes/form.inc).
eigentor’s picture

Can 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.

YesCT’s picture

@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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
5.11 KB
108.23 KB

Re-rolling for admin configuration page with some config file changes.

Status: Needs review » Needs work

The last submitted patch, 1841788-project-browser-120.patch, failed testing.

YesCT’s picture

@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?

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
102.44 KB

@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?

Status: Needs review » Needs work

The last submitted patch, 1841788-project-browser-123.patch, failed testing.

dman’s picture

Just 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

Fatal error: Call to undefined function drupal_get_path_alias() in /private/var/www/drupal8/core/modules/project_browser/project_browser.inc on line 196

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.)

tim.plunkett’s picture

It was included in the change record http://drupal.org/node/1853148, which was the first result.

dman’s picture

Ah 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.

dman’s picture

Issue summary: View changes

Updated issues again

YesCT’s picture

vijaycs85, 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.

sun’s picture

Assigned: sun » Unassigned

I'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.

wildkatana’s picture

Sorry 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!

YesCT’s picture

@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.

YesCT’s picture

we 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)

c4doug’s picture

I 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

  $projects['views'] = array(
    'type' => 'module',
    'title' => 'Views',
    'name' => 'views',
    'drupal version' => 8,
    'author' => 'merlinofchaos',
    'description' => "The Views module provides a flexible method for Drupal site 
      designers to control how lists and tables of content (nodes in Views 1, almost 
      anything in Views 2) are presented. Traditionally, Drupal has hard-coded most of 
      this, particularly in how taxonomy and tracker lists are formatted. ",
    'drupal_versions' => array(6, 7),
    'categories' => array('admin', 'search'),
    'image' => 'http://learnbythedrop.com/system/files/images/View-Edit_0.png',
    'usage' => '542312',
    'project url' => 'http://www.drupal.org/projects/views',
    'project status url' => 'http://updates.drupal.org/release-history/views/7.x',
    'last updated' => '12342523',
    'maintenance status' => 'Actively maintained',
    'development status' => 'Under active development',
    'rating' => '9.6',
    'dependencies' => array(
      'ctools',
    ),
  );

  $projects['ctools'] = array(
    'type' => 'module',
    'title' => 'Chaos Tool Suite',
    'name' => 'ctools',
    'drupal version' => 8,
    'author' => 'merlinofchaos',
    'description' => "This suite is primarily a set of APIs and tools to improve 
      the developer experience. It also contains a module called the Page Manager 
      whose job is to manage pages. In particular it manages panel pages, but as 
      it grows it will be able to manage far more than just Panels.",
    'drupal_versions' => array(6, 7),
    'categories' => array(),
    'image' => '',
    'usage' => '4312',
    'project url' => 'http://www.drupal.org/projects/ctools',
    'project status url' => 'http://updates.drupal.org/release-history/ctools/7.x',
    'last updated' => '12354634',
    'maintenance status' => 'Actively maintained',
    'development status' => 'Under active development',
    'rating' => '7.6',
    'dependencies' => array(),
  );

  $projects['token'] = array(
    'type' => 'module',
    'title' => 'Token',
    'name' => 'token',
    'drupal version' => 8,
    'author' => 'eaton',
    'description' => "Tokens are small bits of text that can be placed into larger 
      documents via simple placeholders, like %site-name or [user]. The Token module 
      provides a central API for modules to use these tokens, and expose their own token values.",
    'categories' => array('admin'),
    'image' => 'http://drupal.org/files/images/token_08.thumbnail.png',
    'usage' => '4563',
    'project url' => 'http://www.drupal.org/projects/token',
    'project status url' => 'http://updates.drupal.org/release-history/token/7.x',
    'last updated' => '12357351',
    'maintenance status' => 'Actively maintained',
    'development status' => 'Under active development',
    'rating' => '8.1',
    'dependencies' => array(),
  );

Themes

  $projects['zen'] = array(
    'type' => 'theme',
    'title' => 'Zen',
    'name' => 'zen',
    'drupal version' => 8,
    'author' => 'johnAlbin',
    'description' => "Zen is the ultimate starting theme for Drupal. If you are 
      building your own standards-compliant theme, you will find it much easier to 
      start with Zen than to start with Garland or Bluemarine. This theme has fantastic 
      online documentation and tons of code comments for both the PHP (template.php) 
      and HTML (page.tpl.php, node.tpl.php).",
    'categories' => array('light', 'dark'),
    'image' => 'http://drupal.org/files/images/zen-logo.thumbnail.png',
    'usage' => '4563',
    'project url' => 'http://www.drupal.org/project/zen',
    'project status url' => 'http://updates.drupal.org/release-history/zen/7.x',
    'last updated' => '12343634',
    'maintenance status' => 'Actively maintained',
    'development status' => 'Under active development',
    'rating' => '7.1',
    'dependencies' => array(),
  );

  $projects['acquia_marina'] = array(
    'type' => 'theme',
    'title' => 'Acquia Marina',
    'name' => 'acquia_marina',
    'drupal version' => 8,
    'author' => 'stephthegeek',
    'description' => "The Fusion base theme and Skinr are required. Skinr for Drupal 7 
      (dev release) is usable now but it is recommended that you proceed with caution 
      and do some of your own testing.",
    'categories' => array('light'),
    'image' => 'http://drupal.org/files/images/acquia_marina.thumbnail.png',
    'usage' => '14563',
    'project url' => 'http://www.drupal.org/project/acquia_marina',
    'project status url' => 'http://updates.drupal.org/release-history/acquia_marina/7.x',
    'last updated' => '12346574',
    'maintenance status' => 'Actively maintained',
    'development status' => 'Under active development',
    'rating' => '7.8',
    'dependencies' => array(
      'fusion',
    ),
  );

  $projects['fusion'] = array(
    'type' => 'theme',
    'title' => 'Fusion',
    'name' => 'fusion',
    'drupal version' => 8,
    'author' => 'stephthegeek',
    'description' => "Fusion is a powerful base theme, with layout and style configuration 
      options built in that you can control through Drupal's UI. It's based on a simplified 
      960px or fluid 12/16-column grid. It's designed to be used with the Skinr module, 
      with numerous useful block styles included.",
    'categories' => array('light'),
    'image' => 'http://drupal.org/files/images/fusion-powering-small-banner.thumbnail.png',
    'usage' => '14563',
    'project url' => 'http://www.drupal.org/project/fusion',
    'project status url' => 'http://updates.drupal.org/release-history/fusion/7.x',
    'last updated' => '12342643',
    'maintenance status' => 'Actively maintained',
    'development status' => 'Under active development',
    'rating' => '',
    'dependencies' => array(),
  );
Bojhan’s picture

Version: 8.x-dev » 9.x-dev

As 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.

:(

wildkatana’s picture

Version: 9.x-dev » 8.x-dev

Sad 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.

YesCT’s picture

Version: 8.x-dev » 9.x-dev
dww’s picture

Update 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...

webchick’s picture

This 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.

cweagans’s picture

+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.

YesCT’s picture

I 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.

sun’s picture

Version: 9.x-dev » 8.x-dev

Since 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:

  1. Create a separate issue to add a new module to core that is initially hidden = TRUE in its .info file.
  2. The separate module ensures that this functionality does not suddenly appear on anyone's site. Because it is always hidden + thus disabled, it's impossible that anyone could have enabled it.
  3. Flesh out the implementation, properly, in all aspects, battle-test it, and commit/add it to core when it's ready for mass-consumption.
  4. If it won't happen for D8 in eternity, then bad luck, but at least there was a placeholder and thus a realistic chance.

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.

David_Rothstein’s picture

Version: 8.x-dev » 9.x-dev

I think it may be premature to move this issue to Drupal 9, for a couple of reasons:

  1. The way I read http://drupal.org/node/1527558, this is not only still on the table for Drupal 8, but even (theoretically) Drupal 7! It is a standalone optional feature that does not affect anything else.

    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.

  2. As per above, the whole concept of Drupal 8 "feature freeze" doesn't actually make sense given that features are still on the table for Drupal 7. And that is already being discussed:
    #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.

David_Rothstein’s picture

Version: 9.x-dev » 8.x-dev

Cross-posted with @sun :)

David_Rothstein’s picture

Also, 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:

  1. Edit core/modules/project_browser/tests/project_browser_test.info and remove the hidden = TRUE line (could even be done in the patch itself, temporarily).
  2. Add code like the following to hook_install() in project_browser_test.install (might actually belong there for good or as default configuration shipped with the module, rather than having the test cases set it up as the code does now):
      $server_url = url('project_browser_test/query', array('absolute' => TRUE));
      $config = config('project_browser.settings');
      $config->set('server.default', array(
        $server_url => array(
          'name' => 'Test Server',
          'method' => 'json',
        ),
      ));
      $config->save();
    

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 :)

falcon03’s picture

@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.

c4doug’s picture

Status: Needs work » Needs review
FileSize
5.21 KB
104.39 KB

It 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.

wildkatana’s picture

@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.

catch’s picture

There'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...

japerry’s picture

Assigned: Unassigned » japerry
Status: Needs review » Needs work

This is fairly out of date, working on updating it to the new routing system and other yml files.

japerry’s picture

For 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

japerry’s picture

Issue summary: View changes

Issue summary added based on template

Leeteq’s picture

Updated, 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...

mdrummond’s picture

Just 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.

mdrummond’s picture

Accidental dup.

webchick’s picture

I 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.

aneek’s picture

@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.

webchick’s picture

I 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.

webchick’s picture

aneek’s picture

Thanks @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.

drumm’s picture

Instructions 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?

webchick’s picture

Were 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.

steinmb’s picture

Assigned: japerry » Unassigned

Un-assigning @japerry. Pls. re-assign if you are active working on this issue.

jhedstrom’s picture

Version: 8.0.x-dev » 8.1.x-dev

Feature request => 8.1.x.

japerry’s picture

I'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.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

sarmiliboyz’s picture

Is 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.

sarmiliboyz’s picture

Project: Drupal core » Drupal core ideas
Version: 8.3.x-dev »
Component: base system » Idea

Can we include this to drupal core ideas so we can make this solid?

David_Rothstein’s picture

Project: Drupal core ideas » Drupal core

What 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...)

David_Rothstein’s picture

Version: » 8.3.x-dev
Component: Idea » base system

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andypost’s picture

Looks this one still should be moved to ideas

catch’s picture

Project: Drupal core » Drupal core ideas
Version: 8.5.x-dev »
Component: base system » Idea
Status: Needs work » Active

Yes agreed, moving it there.

David_Rothstein’s picture

See 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?

David_Rothstein’s picture

Issue tags: +needs backport to D7
webchick’s picture

FileSize
139.28 KB

Yeah, 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:

WordPress Plugin Browser.

drumm’s picture

We 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 pagination page. Adding more is probably okay, as long as it doesn't affect what the Composer CLI requests expect.

gagarine’s picture

Issue tags: -d8ux

Usability is preferred over UX, D7UX, D8UX etc. See https://www.drupal.org/issue-tags/topic

kevinquillen’s picture

I 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.