Ding Code Guidelines

Ding code guidelines

The following guidelines describe best practices for developing code for the Ding project. The guidelines should help achieve:

• A stable, secure and high quality foundation for building and maintaining library sites
• Consistency across multiple developers participating in the project
• The best possible conditions for sharing modules between Ding sites
• The best possible conditions for the individual Ding site to customize configuration and appearance

Contributions to the core Ding project will be reviewed by members of the Core team. These guidelines should inform contributors about what to expect in such a review. If a review comment cannot be traced back to one of these guidelines it indicates that the guidelines should be updated to ensure transparency.

The guidelines cornern Ding 2 but will be taken into consideration when reviewing Ding 1 code.

We acknowledge that not all current code in core complies with these guidelines. We hope that updates to core along the way will move the code base in the right direction.

 

Coding standards

  • PHP
    • Code must be compatible with PHP 5.3. Compatability can be checked using PHP_Codesniffer.
    • Code must conform to the Drupal Coding Standards. In effect this means that the code must pass through an automated review by the Coder module without any notices expect known false negatives. The review must include all severities and all reviews except upgrade7x.
    • Code must not generate notices without erroneous conditions when running in E_STRICT error mode 
  • JavaScript:
    • It is recommended that JavaScript code is checked by JSHint with options: debug, forin, eqnull, noarg, noempty, eqeqeq, boss, loopfunc, evil, laxbreak, bitwise, strict, undef, nonew, browser and jquery. If the web-version is used this equals to checking all warnings and assuming jQuery and Browser environment.
  • CSS
    • Code must pass through an automated review by CSSLint without any notices The following rules are used: display-property-grouping, duplicate-properties, empty-rules, known-properties, box-sizing, compatible-vendor-prefixes, gradients, vendor-prefix, import, zero-units, shorthand, important, regex-selectors, universal-selector, duplicate-background-images.
    • Modules and themes may use SASS + Compass. It is the responsibility of the developers to compile these to CSS files. If the developers uses external libraries (e.i. compass) the version used have to be documented in the scss file.
    • If code is compiled/preprocessed it must be described in the header of the compiled file. If any external libraries are used these have to be documented as well in the header.
    • It is highly recommended that all style-sheets are documented after KSS
  • The default language for all code and comments is English.

 

Documentation

  • README.txt: All modules must contain a README.md file containing the following where applicable:
    • A brief description of the module - preferably with a screenshot
    • Configuration options
    • Installation procedure
    • Hidden variables
    • Other requirements and how to obtain these such as API urls, versions, keys, library system and trimmings
    • Any code which does not comply with these guidelines and a brief argument why
  • LICENSE.txt: All repositories must contain a LICENSE.txt file containing the license for Ding: GPL2.

Naming

General

  • All names must be in English.

Modules

  • All modules written specifically for Ding must be prefixed with either ding or ting e.g.ding_your_module. Which prefix to choose depends on the scope of the module:<
    • Modules dealing primarily with the Open* webservices should be prefixed with ting e.g. ting_search_autocomplete.

    • Modules providing functionality based on library systems or the Ding content model should be prefixed with ding e.g. ding_event.

  • The prefixed are not required for modules which provides functionality deemed relevant outside the Ding sphere e.g. Blackhole and Opening Hours.

Files

Files provide by modules must be placed in the following folders and have the extensions defined here.

  • General
    • MODULENAME.info
    • MODULENAME.module
    • MODULENAME.install
    • includes/*.inc
    • templates/*.tpl.php
    • *.theme.inc
  • CSS:
    • CSS files must use the BAT (Base/Admin/Theme) naming schema as described here.
    • /css/*.(base|admin|theme).css
    • /sass/*.(base|admin|theme).scss
    • /sass/libs/*
  • JavaScript
    • Files must have to include the module name to easily find there origin during debugging
    • /js/*.MODULENAME.js
  • Images
    • Images resources must be placed in a folder named img and have the following extensions.
    • img/*.(png|jpeg|gif)
  • External resources (PHP and JavaScript libraries) must be included using Libraries API version 2.x module

Module elements

Programmatic elements such as variables, content types, views and panel pages, defined in modules must comply to a set of common guidelines.

  • Machine names should be prefixed with the name of the module that defines them
  • Administrative titles, human readable names and descriptions should contain the purpose of the view
  • If a programmatic element supports tagging e.g. rules and views one of the tags must be the name of the module that defines them 

As there is no finite set of programmatic elements for a Ding site these apply to all types except if explicitly specified below.

Guidelines for specific elements:

  • Panel variants (handlers) must not use auto generated names as this increases the risk of different modules implementing variants for the same pages overwriting each other. Instead use module_name_page_name_variant_name e.g. ding_blog_node_view_blog NB: Auto generated names is the default behavior when using features!

Repositories

  • Repositories should names after the module contained within. The repository for the module ding_event should be called ding_event.

Code Structure

  • Each module should be placed within its own repository. Each repository should only contain one module.
  • If a module has dependencies these should be declared within a .make file included with the module so that they are downloaded automatically during a build. Dependencies can be projects on Drupal.org or external resources. It is recommended that dependencies specify a specific version.
  • A module should provide all required code and resources for it to work on its own or through dependencies. This includes all configuration, theming, CSS, images and JavaScript libraries.
  • All default configuration required for a module to function should be implemented in code. The preferred way of doing this is using the Features module or hooks provided by the module itself. Modules where configuration cannot be handled using features should maintain configuration using hook_install() and hook_update_N().
  • If a module requires configuration for which there is no sensible default the module must implement hook_ding_install_tasks() such that users can perform the necessary configuration during installation.
  • All default text content in modules must be in English. Localization of content must be handled using the Drupal translation modules. 

Updating core modules

  • If a core module is expanded with updates to current functionality the default behavior must be the same as previous versions or as close to this as possible. This also includes new modules which replaces current modules.
  • If an update does not provide a way to reuse existing content and/or configuration the update then the decision on whether to include the change resides with the Ding team. If the Ding team can not reach an agreement the Ding Council will make the decision.

Altering existing modules

  • Modules which alter or extend functionality provided by the core modules should use appropriate alter hooks to override these instead of forking these modules.
  • In cases where modules provide layered configuration modules should implement new layers with a higher priority than default configuration instead of altering the default configuration. This is often handled through lower weights. To support multiple modules providing separate layers of configuration without conflicts identifiers must be unique to the module as opposed to auto-generated. This may require altering auto-generated code manually. The recommended way to generate unique identifiers would be to prefix the existing identifier with the module name. Example: Panel variants.
  • It is preferable to add a new layer to layered configuration instead of altering an existing layer provided by the core modules. This ensures that alterations and extensions work as expected even when the core modules are updated. This means that sites which use alterations may not reflect the latest changes to core. It is deemed preferable instead of potentially breaking the site.

 

Kommentarer

Foreslag til ændring af CSS guidelines

Jeg vil foreslå at vi fjerner reglen om box-sizing, da grunden til at have den er at det ikke er understøttet i IE 6 og IE 7.

The intent is to ensure that developers realize this property is not supported in older browsers such as Internet Explorer 6 and 7.

IE7 understøttelse

Mener du at ding2 ikke skal understøttes af IE7? Så vidt jeg er orienteret, sidder de fleste biblioteker stadig med IE7.

Box-sizing polyfill

Hvis ellers ikke polyfills er absolut bandlyste, så findes der ét her.

Box-sizing gør i hvert fald mange ting meget lettere, så det ville være rart, hvis det kunne bruges.

da grunden til at have den er

da grunden til at have den er at det ikke er understøttet i IE 6 og IE 7

Jeg er enig med Guddo her. Grunden til den er der er som en advarsel tid udviklere, der måske kan have en tendens til at checke deres design op imod specifikke browsere med bedre CSS understøttelse end andre og derfor ikke får checket i IE6/7. 

Der kan sagtens være situationer, hvor der både bruges box-sizing og tages højde for manglende understøtte gennem browserspecfikke style sheets. I sådanne tilfælde koden kunne passere et review på trods af disse advarsler.

Personligt synes jeg polyfills lyder som et interessant alternativ og hvis det bliver accepteret ifølge Code Guidelines kunne kan også fjerne denne regel.

IE6 og IE7 m.m.

For det første vil jeg mene at vi overhovedet ikke skal bruge tid på IE6, er der nogen der er uenige i det?

Jeg er enig med Bech, og vil gå så langt, som til at sige at box-sizing er et must. Latto bruger box-sizing og det polyfill du nævner i gridsystemet (http://zengrids.com/ lavet af John Albin).

IE7 er en anden snak, men i følge http://fdim.dk/statistik/teknik/browserbarometer er den nede på 2% i juni 2012, i følge analytics på aakb er 5,52% fra IE7, og tallet er faldende.

I følge Dries til DrupalCon München kommer Drupal 8 ikke til at understøtte IE6/IE7.

Min anbefaling er at vi understøtter IE8+ (og selvfølgelig Firefox og webkit), og droppe support for de gamle browsere.

Vi opnår bl.a.:

  • At få folk til at opdatere deres browser, og hjælper dermed til at få slået IE7 ihjel.
  • Vi sparer tid i forhold til browsertesting, både nu og løbende når der kommer nye features.

Hvis der er et bibliotek der har behov for IE7 understøttelse, kan man jo lave de tilretninger der skal til i et IE7 stylesheet i sit eget tema.

Så for at opsummere: Jeg mener ikke at vi bør understøtte og bruge tid på en browser der har < 10% (formentlig mindre) af markedet og er på vej ud. Det sparer os tid både nu og fremadrettet.

IE7 på bibliotekerne?

Hvis der er et bibliotek der har behov for IE7 understøttelse, kan man jo lave de tilretninger der skal til i et IE7 stylesheet i sit eget tema.

Her mener jeg det er et spørgsmål om målene for Ding2tal og realiteterne ude hos bibliotekerne.

Hvis det er som Guddo siger og mange biblioteker sidder fast på IE7, så mener jeg det er problematisk hvis Ding2tal ikke understøtter det. Bibliotekerne har sandsynligvis ikke selv mulighed for bare at opdatere, og det vil være ærgeligt hvis de hver især skal lave de samme basale tilretninger i hver deres tema.

Hvis vi skal supportere IE7 *og* gerne vil spare ressourcer, mener jeg polyfill'et må være den mest pragmatiske løsning.

Enig

... Bibliotekerne får sandsynligvis også klager, hvis IKKE ie7 er understøttet. Men når nu, der eksisterer et polyfill, er det vel ikke et problem? :)

Polyfill vil nok løse nogle

Polyfill vil nok løse nogle problemer men tvivler på det er alle. Jeg tror det skal besluttes på et højere niveau, og der skal evt. undersøges hvor meget IE 7 reelt fylder landet over.

Man kan så understøtte det

Man kan så understøtte det med en pæn "Opdatér din browser" side.

Chrome Frame?

Et andet alternativ til "Opdater din browser" kunne være Chrome frame. Det er der selvfølgelig også et Drupal modul til. Jeg ved dog ikke hvilke tilladelser, det kræver at installere plugin'et.

Yup!

Yup!

Field machine names kan ikke prefixes

I guidelines står det at: "Machine names should be prefixed with the name of the module that defines them"

Men via field UI er det ikke muligt at prefixe et machine name på et felt. Så guidelines bør indeholde en option for at et machine name på indeholde suffix med modul navn.

Så et machine name for et felt kommer til at hedde: field_ding_!feature!_!fieldname!

Ungarsk notation

Det er rigtigt at man ikke kan gøre det via UI'en, men det kan heldigvis fixes ved at lave en search & replace på den eksporterede feature. Jeg er ikke specielt vild med ungarsk notation, så hvis vi kan slippe for field_ prefixet vil det være at foretrække, imo.

Jeg er heller ikke fan, man

Jeg er heller ikke fan, man ender hurtigt med en masse felter ala field_ding_mobile_images_image, med redundancy hvor vi nærmer os 32 tegns grænsen.

Derudover er konsekvensen også felter der ellers var delt imellem node typer pludselig bliver separate felter, så themingen pludselig ikke bare skal tage stilling til field_image men field_ding_news_image, field_ding_article_image, etc.

Der er ikke rigtig nogen magic bullet der tager hensyn til alle problematikkerne.

Delte fields i et delt modul

Derudover er konsekvensen også felter der ellers var delt imellem node typer pludselig bliver separate felter, så themingen pludselig ikke bare skal tage stilling til field_image men field_ding_news_image, field_ding_article_image, etc.

Denne guideline forhindre jo ikke at ding_news og ding_article begge indeholder instanser af det feltsom er defineret i et modul, som de begge depender på fx. ding_content.

Jeg kan godt se argumentet i

Jeg kan godt se argumentet i forhold til 32 tegns grænsen. Men ellers ser jeg det som endnu en ting der skal tages højde for i udviklingen. Det er allerede et kompliceret workflow/setup (branching, features osv.) så jeg vil helst undgå at komplicere det yderligere.

Og hvis det andet argument blot er navngivning (kosmetisk?), så synes jeg ikke det har nogen værdi for brugere eller udviklere.

Men igen, så er jeg jo bare dødelig frontender, så der er garanteret noget jeg ikke kan se/har overset. :)

En anden ting: Hvordan reagerer Drupals UI i forhold til felter der er ændret i koden? Hvad hvis et modul (place2book f.eks.) skal koble felter på en indholdstype?

Ang. deling af felter, så er de fleste felter jo unikke for en indholdstype for ikke at skabe afhængigheder og fælles indstillinger på tværs af indholdstyper.

Med hensyn til theming så er feltnavnet ikke noget problem, det bliver ikke brugt til styling.

Commits of features

Maybe add something like this:

Features make it very easy to include more changes than intended, so pay extra care to what is actually changed in the diff.

When making order changes, like reordering field in content types, keep the order change in a separate commit comprising only of the order change.

Order changes make it very hard to figure out what is happening if you include other changes in the commit.

Commits of features

Actually the code is only reordered on field renames, not when reordering the fields. So the above should read ..., like renaming fields in content types (features exports are alphabetically ordered), ...

Godt input

Jeg synes dette er værd at få med, men jeg kan af uransagelige årsager ikke redigere wiki'en.

Jesper?

Først manuelt, så eksport med features

Med hensyn til omdøbning af feltnavne, så vil jeg anbefale at man ikke gør det gennem GUI, men at man i stedet omdøber felterne i filen i et commit og så loader featuren og exporterer den (features vil så sorterer indholdet alfabetisk) og gemme det i et nyt commit.

På den måde vil man få alle indstillingerne overført fra det gamle feltnavn over i det nye feltnavn. De resulterende commits vil også være væsentligt mere overskuelige, med funktionsændringen og kodestilsændringen hver for sig.

Du har ret i denne antagelse

Du har ret i denne antagelse og i forbindelse med ding2tal har vi ikke gjort det 100% optimalt.

Jeg ved det er verdens dårligste grund, men det skyldes at vi er mange mennesker og ikke alle vil kunne gøre det via koden og da selve ændring af felt navne i ding2tal foregik under meget stor tidspres i forhold til deadline 01/10/12... så jeg/vi beklager de side effekter det har givet.

Opdatering af guidelines

Jeg er ikke helt sikre på hvor dette vil passe ind og bør vi ikke samle ting op også lave en ny revision af vores guidelines. Synes ikke det er en god ide at tilføje løbende uden nogle revisions kontrol etc.

Måske passer det her:

Jeg oprettede denne wiki: http://ting.dk/wiki/ding2tal-structural-choices - som måske kunne være et godt sted at skrive sådanne praktiske ting som praktisk ændring af feltnavne. Wikien var tænkt som et arbejdsdokument, hvor vi kunne samle sådanne ting op (der ikke som sådan kan siges at være coding guidelines).

Code guideline tips

Da det har generel anvendelighed kunne man måske lave en mere åben wikiside hvor folk kan bidrage med tips til overholdelse af Code Guidelines?

Noget af det måske

Jeg synes der skal stå noget i guidelines omkring commits, men hvis der er mange konkrete tips til hvad man kan gøre for at overholde de guidelines for commits, så passer det ind i sådan en wiki.

Nyt punkt

De nuværende guidelines tager kun hensyn til resultatet ikke versionsstyringen som skaber resultaterne.

Jeg synes det skal ind under et nyt punkt. Det er vigtigt for mig at de commits der accepteres er læsbare. At man kan følge ændringer tilbage til deres oprindelse og kan reverte ændringer. Derfor synes jeg også der skal være guidelines for commits. Altså bland ikke ikke-funktionelle ændringer og funktionelle ændringer, bland ikke flere funktionsændringer sammen i et commit. Dertil kommer så et konkret eksempel, som jeg har beskrevet.