Posted by webchick on August 4, 2011 at 9:37am
Back at DrupalCon Chicago, Dries outlined a strategy for Drupal 8 involving a series of "gates" that would help ensure core code quality in a number of different categories: Documentation, Performance, Accessibility, Usability, and Testing. Patches to Drupal 8 would be required to pass these in order to be accepted (as well as pass standard human reviews).
The idea behind the gates is to define essentially a set of "checkboxes" which outline the most important aspects to each category, and does so in a way that is both not overwhelming to core developers (list of hoops to jump through is kept as small/focused as possible) and also creates very little additional burden on the team in question (contains ample links to documentation/resources so developers/reviewers can help themselves).
The documentation, accessibility, performance, usability, and testing teams have been hard at work on defining their gates over the past month, with a focus on trying to identify the highest-impact items and help raise the collective IQ of the development community by educating them on how to ensure their patches pass. While these gates (apart from documentation) are NOT quite finalized yet, several teams have asked for additional community feedback. The goal is to get these finalized within the next 2 weeks in order to formally announce them at DrupalCon London, where lots of sprinting on D8 is sure to be happening.
Items that need review:
- Are the gates clear in what they're asking for?
- Is it also clear when a gate would apply to one of your patches, or to a patch you're reviewing?
- Are there ample resources to help you pass the gate?
- Any other comments?
So, without further ado, the gates!
Note: If you have bandwidth to help the teams push these through the finish line (helping to identify/write missing documentation, helping with wordsmithing on the actual gate text to make them more clear, etc.), this would be greatly appreciated. :)
Note: http://drupal.org/node/1203498 is the final home these will live in when they're ready.
Documentation
| Description | When is this needed? | Details | Resources |
|---|---|---|---|
| Issue summary is created | Required for any issue that cannot be understood easily at first glance. Recommended for all issues. | Edit the original issue node to summarize the issue, using the template. | Template: http://drupal.org/node/1155816 |
| In-code API documentation is added or updated | Required for all patches that change or add to code behavior. | All functions, classes, files, constants, etc. that are added or updated must be documented. | Standards: http://drupal.org/node/1354 |
| module.api.php doc is added or updated | Required for all patches invoking new hooks or changing how hooks are invoked. | All new and updated hooks invoked by the code (including drupal_alter() calls) must be documented. | Standards: http://drupal.org/node/1354#hooks |
| hook_help() doc and module description are added/updated | Required for all patches that add modules or change the user interface. | Standards: http://drupal.org/node/632280 | |
| Change notification | Required if the patch makes API or user interface changes that module or theme developers need to know about, or that will need to be documented outside of the patch. | After commit, API change and User interface change sections from the issue summary are added as a change node. (Change nodes will track additional developer/themer documentation that needs to be updated, in turn.) | Use tag “Needs change notification” to indicate the change node(s) have not yet been created. |
Accessibility
View/join discussion here: http://groups.drupal.org/node/158759
| Description | When is this needed? | Details | Resources |
|---|---|---|---|
| Conforms to WCAG 2.0 and ATAG 2.0 | When changing the UI. Common accessibility issues and tests are listed below. | The goal for Drupal core is Level AA conformance with WCAG and ATAG 2.0. Use existing UI patterns to minimize the opportunity for accessibility or usability problems. Tag issues that introduce new patterns with "needs accessibility review". | WCAG 2.0, ATAG 2.0, Drupal Accessibility Statement, and More about Drupal core conformance and UI patterns |
| HTML structure meets WCAG 2.0 | When introducing new HTML structure or semantics, such as when changing templates or theme functions. | Run an automated checking tool like the WAVE Toolbar. Test before and after a patch to verify that the patch does not introduce new errors. | WAVE Toolbar and More about Automated Checking |
| Text color has sufficient contrast | When introducing new foreground and background combinations, such as when changing CSS. | Run a contrast test such as Contrast-A to check and repair color combinations. | Contrast-A and More about Contrast |
| Form fields are labeled | When adding forms or changing fields, such as when using Form API. | Every form field needs a correctly associated label. Run a test using the WAVE Toolbar (see above) on any new or changed forms and fix any incorrect labels. | More about #title and #title_display |
| Javascript is keyboard-usable | When developing new Javascript UI interactions or AJAX widgets. | Test using only your keyboard (unplug your mouse). Verify that the UI can be fully operated using only a keyboard. | More about Javascript accessibility |
Usability
View/join discussion here: http://groups.drupal.org/node/158764
| Description | When is this needed? | Details | Resources |
|---|---|---|---|
| Drupal's design principles are followed. | Any patch that changes the user interface. | Design consists out of a lot of different guidelines, ideas and principles. Follow the overarching principles to create a delightful experience. | Discussion |
| User interface guidelines are followed, and new patterns are documented. | Any patch that changes the user interface. | By following the user interface standards we create a more consistent experience. Provide a strong rationale when you deviate; and document new interface patterns in the library. | UI standards |
| Make it easy for people to give design feedback. | Any patch that changes the user interface. | Follow common practices, to allow for feedback from non-developers. By supplying screenshots and rationale on how your change affects the user. | Process how-to |
| Verify your solution with users by performing usability testing. | Any patch that makes major changes to the user interface. | Usability testing is an important tool for determining the benefit of a proposed change to the user interface. For major changes it is required to verify the user experience of your solution with users. | Discussion |
Performance
View/join discussion here: http://groups.drupal.org/node/158769
| Description | When is this needed? | Details | Resources |
|---|---|---|---|
| SQL Queries | If there is an additional, new or changed SQL query in a patch | When a patch adds database calls (including indirect calls via API functions), the issue should include an indication of how often the query is likely to run. If queries are added or modified that are not identical to existing queries, then additionally EXPLAIN output should be posted to ensure they use indexes as well as possible. The goal is to avoid adding additional database calls unless necessary, and ensuring that queries scale when using large data sets. | http://drupal.org/project/devel / http://dev.mysql.com/doc/refman/5.0/en/explain.html |
| Memory | When large arrays or objects are added or modified, lots of PHP files are included, or when static caches are added. | Loading large arrays or objects into memory, including a very large number of PHP files, or adding static caches can all affect memory usage negatively. Issues should be accompanied by before/after xhprof screenshots or similar data detailing the impact of these changes. | xhprof / (devel) |
| CPU | If a patch adds or changes any code that is frequently executed during a 'normal' request to Drupal, especially up to and including full bootstrap, or during the page rendering process | Ideally post before/after xhprof or cachegrind detail for the area of code being modified. If the change is to a single function, 'unit benchmarks' may be appropriate. Performance results from load testing tools such as ab/siege/jmeter are discouraged since they are often testing hardware rather than the application and are easy to get wrong. | xhprof / xdebug |
| System calls | If a patch adds or modifies include/require, file_exists()/file_mtime(), time() calls during normal operation or cache rebuilds. This does not include simply adding new files or modules, however it applies to refactoring of any centralized loading or scanning functions. | Post before/after xhprof, cachegrind or strace detail for the area of code being modified. | http://en.wikipedia.org/wiki/Strace / xhprof / xdebug |
| Caching | If patch adds, removes or modifies static or persistent caching. | Caching is a trade-off between memory, CPU, storage and developer experience. Before/after xhprof screenshots are recommended to show the impact of changes made. Changes that modify or add large 'rebuild' operations should post similar performance data regarding cache misses. | |
| Front end | If a patch makes significant changes to JavaScript or CSS. | If refactoring commonly used JavaScript, post profiling data. If changing JavaScript and/or CSS aggregation logic, size and number of aggregates across several page requests should be posted to the issue | http://www.webpagetest.org/, google chrome toolbar, firebug, speedtracer |
Testing
View/join discussion here: http://groups.drupal.org/node/158774
| Description | When is this needed? | Details | Resources |
|---|---|---|---|
| Check test coverage and ensure all tests pass | When changing/refactoring existing code | When making changes to existing code, ensure the code being worked on has test coverage. This will be the case if some tests fail during the course of working on the patch, so may not need to be checked manually. No patch can be marked RTBC if there are outstanding test failures, since core has a 100% pass rate policy. | http://qa.drupal.org |
| Add new tests | When adding new features | When new features are added, they should be accompanied by automated tests. If the feature does not have an implementation, either unit tests, or a test module that demonstrates the functionality can be provided. | http://drupal.org/simpletest |
| Upload a test case that fails | When fixing bugs in PHP code | Bug fixes should ideally be accompanied by changes to simpletest (either modifying an existing test case or adding a new one) that demonstrate the bug. This can be shown by uploading a patch with only the test changes, which Drupal.org's automated testing system should show as a fail, alongside a patch containing both the tests and the bug fix itself. If a bug fix is not accompanied by tests, there should be an explanation in the issue for why no tests are needed (usually only for trivial bugs where testing would be convoluted) before it is marked RTBC. Patches that need tests written can have the 'needs tests' tag added. | http://drupal.org/simpletest, http://qa.drupal.org, http://drupal.org/project/issues/search/drupal?status[]=Open&version[]=8.x&issue_tags=Needs+tests |
| Manually test in browsers and provide screenshots or screencasts | When making markup, CSS or JavaScript browsers | Drupal core does not have automated acceptance or unit testing for non-PHP code. In these cases changes to CSS, markup or JavaScript should normally be accompanied by confirmation that this has been tested in the browsers Drupal core currently supports. For large changes or bug fixes, screenshots or screencasts are ideal for demonstrating the impact of the changes. | ... |
| Provide an example module | When a new feature is not implemented by Drupal core | When adding major new features that are not implemented by core (i.e. API-only features), example code should be provided that demonstrates the feature. This may take the form of a test module, but could also be a contrib sandbox or project, or follow-up core patches that introduce the functionality (and can be tested as proof of concept prior to committing the feature). |
Comments
What about adding yet one
What about adding yet one category - Integration
FE: Magento (as backend), OpenERP (as backend), CRM (many) and many others
Drupal can be good as Client for many client-server technologies
Andriy Podanenko
web: http://druler.com
That sounds more like a
That sounds more like a feature request than a gate.
Integration mostly depends on
Main troubles are collisions
- namespace
- class
- globals
We should add a list of
We should add a list of browsers needed for manual css/js testing. I propose this list.
IE7, IE8, IE9, chrome or safari, firefox, opera
You can test IE7-IE9 easily with the "internet explorer platform preview". There are options to switch between those versions without installing 3 versions of IE.
Chrome and safari are build on the webkit render engine so in 99,5% procent of the cases (just invented that number) they will look identical. I prefer chrome because it's known to incorporate bugfixes faster.
I consider testing in safari as a plus.
+101 IE7+ unfortunately, we
+101
IE7+
unfortunately, we can't kill IE7 :(
Chrome
Auto-force update system in Chrome, so we can only test stable version, windows preferred.
Safari
current and one previous major releases (Mac OSX only)
and iPad ? iPhone ?
Firefox
supports 5.0+
Opera
one of top browsers in mobile world
There's a lot more to Front
There's a lot more to Front End then filesize changes to css/js. Proper expirations on cacheable items, use of CSS sprites, optimized images, efficient CSS selectors, etc should be included in this gate.
I am wary about the
I am wary about the performance gate containing specific recommendations for code/patch quality since those are subject to change and/or can depend on context (it does not recommend particular kinds of database queries or PHP code patterns for the same reason). So want to be careful how we word that, this applies to mentioning JS/CSS bundle size too so mea culpa.
Optimized images - yeah that sounds good for new images.
Expiration on cacheable items - we have .htaccess rules for this and max_age support for proxy caches for cached HTML, but short of a patch to remove the feature altogether I'm not sure that is going to come up much in core patches?
CSS sprites - core would fail this currently. Core would also fail CSS selectors - until the drop IE6 patches get in.
So how about something like:
changes that add or modify images, JS or CSS should be accompanied by the impact on file size and number of http requests on the pages they will be rendered.
Patches that change HTTP headers for cacheable items should be accompanied by header output from a request or a clear explanation of the likely effect of this change on CDNs and proxies (or is there a better way to word this?)
Changes to CSS and JavaScript should be accompanied by an explanation of the likely impact on performance in terms of browser rendering times. Ideally accompanied by data (for example by comparing approaches using jsperf.com or other appropriate tools).
Any better?
"Consistency"?
How about an item for indicating consistency? How many API patches were not included in D7 to fix API inconsistencies, e.g. missing tokens, missing drupal_alter() calls, missing hooks, etc? D8 needs to be much more well rounded in this regard, there shouldn't be any cases of things missing like this.
Unit testing
I'm not sure how exactly to make this a "gate", but we should strive to not use DrupalWebTestCase if we can avoid it. Any code that can should be using DrupalUnitTestCase. Not only is it about 1000x faster, it also means we're testing the specific code in question rather than a complete Drupal instance, which gives a better picture of if the code is actually working as well as if it's properly written, that is, properly encapsulated and not integrated into Drupal's dependency spaghetti.
gates and actual code quality
I think this is another one where the purpose of the gates is a bit undefined, it's one or both of these:
making sure there is sufficient information in the issue for reviewers to be able to quickly tell if something important is missing (and defining what 'something important' is).
actually specifying a quality threshold for particular areas of certain patches (which something like 'unit tests preferred to DrupalWebTestCase') is.
It's my own view that we can't do the latter with the gates. We could add 'unit tests preferred' to a best practices guide, and reviewers can refer to it (a bit like code style or the hig), but it feels hard to define for a gate - if I'm just updating an existing test, I would not want to be required to convert it to a unit test for example.
Also with actual code quality, and particularly unit tests - a lot of core code fails the current standards that we have or would like to have.
Obviously some people would like to see code quality metrics included in the gates, going by comments here and elsewhere, but that feels like a wider discussion about their purpose in general (and how they'll be enforced) - which is partly what I'm hoping this general issue will encourage.
Can they already use objects?
Last time I tried it, unit tests couldn't use any Drupal classes, as database access was completely blocked, disabling autoloading. For me, this made all unit tests impossible (cf. issue #883264), and I can imagine lots of core code that would be very well-suited for unit testing lives in classes, too.
If autoloading still can't be used in unit tests, maybe we should fix that. As you say, this could bring a huge performance gain to our tests.
Registry
The registry is blocked, as that requires a Drupal environment. I don't see why a require_once() on the files you need would be an issue, though.
Dependencies
Because of all the dependencies, of course. You don't just have to load the used classes, and all their parents, but also all classes used in the tested methods, etc. This really is neither a stable nor a comfortable solution.
Bug
If your class has 15 hard dependencies on another class, rather than taking injected interfaced objects, then that's exactly the sort of design flaw we should be avoiding. :-) If you cannot test such a class with DrupalUnitTestCase, then arguably it should be refactored such that you can. That's the whole point.
XHProf integration
It may be helpful if we can put some XHProf init code into Drupal that only runs if a given GET parameter is passed. Given that it writes to the disk I'm not 100% sure we can do that, since everyone's dev box is different, but it would be much easier to get XHProf results and post them if we could eliminate the "edit core to add your site's custom xhprof bootstrap code in somewhere reasonable" step. (I am fairly new to xhprof myself so if there is a solution to this already that I don't know about, please share.)
Sadly it doesn't work on a
Sadly it doesn't work on a windows machine. Isn't there any other profiling software that works on every OS?
xdebug should work on
xdebug should work on windows. Anywhere there is a requirement for xhprof output, it should also mention xdebug profiling. It is a bit harder to get memory information from xdebug (it's there but it's not exposed in any UIs that I know of), and it only shows wall time rather than wall time vs. cpu time, but otherwise they more than close enough - especially compared to no profiling at all.
Devel
Devel has this baked in now, I just used it today and it works really well.
sharing :)
There is xhprof support in devel module (although i've been meaning to submit a patch that allows starting profiling from sites.php or settings.php - currently it can only start from hook_boot().
Also take a look at http://drupal.org/project/xhprof which has two purposes (afaik, better to speak to msonnabaum than me):
building a native Drupal UI for xhprof - there is a nascent views backend that might not be committed yet (the code for the html UI that ships with xhprof is functional, but it is also shockingly bad code and not re-usable)
integration with simpletest so you can write 'unit tests' and also functional tests and record profiling data for them automatically. Justin Randall has been working on that.
For more discussions around this, there is http://drupal.org/sandbox/catch/1186744
However for the performance gate itself, we are going to need to start off with manual testing and add automated tests later - hopefully we can get it into a state where that can be added to the gate (or at least make it easier to get the data for the gate)
JS Fallback?
There is a requirement that Javascript is keyboard-usable. What about browsers that have JavaScript disabled?
I propose that user-facing JS functionality needs to provide a usable non-JS fallback. I would recommend but not require that for administration pages.
Upgrades
Is there any way to include some kind of safeguards against broken upgrades like we currently have on the node language fiasco?
Nancy Dru
Issue summary template - add fields to issues?
The template at http://drupal.org/node/1155816 is fine, but wouldn't it be better to have the sections as fields?
This would be a better guide for issue authors, and will be compatible with future changes we may want to introduce.
Too complex/rigid
a) Would require some config/programming to get the fields onto the Issue content type
b) we don't actually want the issue summaries to be that rigid. Sometimes a different structure is more appropriate for a particular issue.
Drupal programmer - http://poplarware.com
Drupal author - http://shop.oreilly.com/product/0636920034612.do
Drupal contributor - https://www.drupal.org/u/jhodgdon
FrontEnd performance with WebPageTest.org
What about actual end user performance testing with WebPageTest.org.
Scripts can be easily written which will spider a lot of common pages. With or with out cache. And results exported afterwards.
WebPageTest.org also takes browser screenshots, which would help with UI debugging on different browsers. Currently only IE6, 7, 8, 9 and Chrome are available, but with some work (or sponsorship) could be could be expanded to other browsers (safari probably easily, firefox probably harder).
I know they allow you to run your own instances of their test suite, so long as you make it available for others to use. I have a friend who is a partner with them and runs an amazon micro instance at the cost of roughly $30 a month out of Singapore.
I would also recommend making some install profiles which use common contrib modules + themes (Views, Token, EntityAPI, Zen, Acquia Marina) and benchmarking core against these as well. Make like 2-3 common use cases install profiles and populate them with a standard amount of data. These might help identify real world problems with core & identify backward compatibility issues. When creating the profiles, use current versions of those modules, then leave them on those versions so that comparing benchmarks are relevant.
Ideally these world be real world installation profiles like Ubercart, Drupal Commerce, Atrium, OpenPublish, but that would depend on those being ready when D8 is. Wouldn't hurt to add those into the mix when they are ready though.
Drupal & Commerce Themes
Not a gateway
I don't understand how "Make it easy for people to give design feedback." is an actual gateway. This seems more of a community issue rather than a gateway/checkbox for a particular patch. I"m not suggesting its not something for us to do / champion, but its not a gateway.
It would also be worth while having some minimum standards for the usability testing. Is 2 ppl enough? 10? 50?
While this will undoubtedly increase the quality of the patches that get submitted, it also severely increases the barrier to entry on submitting a change. Aside from documentation, are there going to be any efforts around reducing the effort to meet/test patches against these gates?
Some examples that come to mind:
- An additional field(s) on issues to track what gates it has passed and/or are still needed. Tags are flexible but having a more formal set of standards seems like a more structured approach will help. (I agree with Jenn about the fields for the template summary)
- Integrating any of these gates (particulary the security or accessibility ones) with our automated testing infrastructure.
It is also unclear to me if this applies to ALL patches for D8? or just to the bigger ones for initiatives?
Possibly a few missing checks, but maybe they are already a given:
- passes coding standards /coder module?
- upgrade path?
- security? i.e. running against an automated penetration test?
- proper support of locale/i18n
- namespace usage/conflicts
- extensibility?
--Ryan
Ryan Cross
Drupal Development Services
ProjectPier project management and collaboration software
I think a Security gate is necessary.
Webchick, thanks for your continued community "shepherding," and of course to Dries for the gates idea. To ensure core code quality, I'm going to argue that a Security gate is necessary.
As with accessibility, performance, usability, and testing, Web application security is much easier for us to incorporate into our assumptions about architecture, and to put in the code itself, as it is being added to core, rather than "bolting it on" afterward. Plus it's more time- and cost-efficient, as you've no doubt heard.
I agree that the "collective IQ" needs to be raised, as you say. Wouldn't you agree that the basic principles of writing secure code constitute a low-hanging fruit? Plus, the Security Team surely can't and shouldn't do it all. We need to help Drupal's contributors remain aware of the impact of well-though-out, secure code -- and of its opposite.
One of the reasons that we're pleased to use Drupal on our "dot gov" sites is due to the confidence we have in its security foundations / "posture." First, it is open source software. The Department of Defense has pointed out that making source code visible greatly aids defenders and not just attackers. More than that, Drupal has an active and visible Security team, public guidelines for writing secure code, responsible disclosure policies for vulnerabilities, and an API that helps overcome some of the common shortcomings in our code. These are good signs for folks interested in security. However, if we have a gate in place that keeps good security in our thinking, and makes it part of our everyday practice, we'll be more likely to see secure code get in to core. That'll help us all to maintain the confidence and trust that is building around Drupal, and thereby increase its adoption.
I wasn't able to attend DrupalCon London so I don't know if this was already discussed in person. Whaddya think? Also thanks again. :)
Got some feedback from webchick on this
FYI regarding this gate suggestion, webchick sent me some feedback in public IRC today that since 1) all patches to core are checked for security issues as a matter of course, 2) security issues with submitted core patches are pretty uncommon, and 3) automated security tools currently under development might help with this, it does not appear that a formal gate will be necessary.
As the gates are issue
As the gates are issue related and need some issue state change I created a dreditor patch dying for review to use Macros and Templates for better issue management.
The gates page http://drupal.org/node/1203498 explains what to do as an issue reviewer.
I tried to translate that into what to change of the issue being reviewed by the reviewer through http://drupal.org/node/1280562. I.e.
- The issue needs a tags added or removed.
- The issue state must be set to needs work.
- The issue owner needs some pointers to solve the points made.
This page (and others) is then used by dreditor patch (http://drupal.org/node/1115636) to help the issue reviewer by a 'one click' solution.
A demo http://www.youtube.com/watch?v=vpyZu9467yc is a little speedy and already outdated but shows hopefully the idea.
Some question:
- is this gates related issue state change needed?
- what tags are needed / defined?
- should we beef up http://drupal.org/node/1203498 by adding issue state changes? This way we now what tags are defined for what (sub)gate.
Cheers,
Clemens