Working issues

Background

There are two initiatives working on default content and new default theme designs. These are being worked on in parallel and should be tested together instead of separately. This will allow us to get as realistic feedback as possible about both initiatives.

The problem

It is not feasible to modify existing installation profiles for experimental purposes since lot's of people use them on production.

Proposed solution

Create experimental installation profile which allows demonstrating multiple experimental features together using the umami food magazine scenario as a demo. The umami demo will feature default content including articles, recipes, taxonomy and menus as well as the new umami theme.

The installation profile should be shown on the installer and marked as "new" to encourage testing. We should work with the DA to see if we could get statistics of usage from them.

Current installation profile work:
https://github.com/gareth-fivemile/demo_umami

Current composer build for profile plus theme:
https://github.com/gareth-fivemile/umami-build

Current dev version of umami theme:
https://www.drupal.org/project/umami

Previous work by Larowlan:
Sandbox: https://www.drupal.org/sandbox/larowlan/2808575

commit credits

Extra user names for the commit message:
See comment #64 for context.

  • kjay
  • ckrina
  • thamas
  • markconroy
  • mariohernandez
  • big_man
  • budda
  • eli_t
  • john-cook
  • Paul_Gregory
  • Thatdamnqa
  • philipnorton42
  • andrewmacpherson
  • nathancz
  • navneet0693
  • jaykandari
  • thamas
  • mariohernandez
  • sharjay
  • lauriii
  • tomphippen
  • vijaycs85
  • tim.plunkett
  • jibran
  • Tarun Lewis
  • Petr Illek
  • kreynen
CommentFileSizeAuthor
#197 Screen Shot 2018-01-18 at 3.17.08 PM.png205.04 KBwebchick
#197 Screen Shot 2018-01-18 at 3.14.07 PM.png292.8 KBwebchick
#197 Screen Shot 2018-01-18 at 3.13.29 PM.png166.77 KBwebchick
#194 Screen Shot 2018-01-18 at 12.21.49 PM.png108.47 KBwebchick
#194 Screen Shot 2018-01-18 at 12.28.09 PM.png399.52 KBwebchick
#194 Screen Shot 2018-01-18 at 2.06.47 PM.png540.14 KBwebchick
#194 Screen Shot 2018-01-18 at 2.04.48 PM.png665.94 KBwebchick
#194 Screen Shot 2018-01-18 at 2.03.44 PM.png746.94 KBwebchick
#194 Screen Shot 2018-01-18 at 2.02.42 PM.png1.23 MBwebchick
#194 Screen Shot 2018-01-18 at 1.58.12 PM.png1.34 MBwebchick
#194 Screen Shot 2018-01-18 at 1.54.55 PM.png1.08 MBwebchick
#194 Screen Shot 2018-01-18 at 1.53.46 PM.png347.27 KBwebchick
#194 Screen Shot 2018-01-18 at 1.52.38 PM.png449.65 KBwebchick
#194 Screen Shot 2018-01-18 at 1.48.31 PM.png1.54 MBwebchick
#179 add-umami-profile-to-drupal-core--2809635-179.patch3.17 MBnavneet0693
#179 interdiff--2809635-174-179.txt2.75 KBnavneet0693
#174 add-umami-profile-to-drupal-core--2809635-174.patch3.17 MBnavneet0693
#174 interdiff--2809635-171-174.txt2.82 KBnavneet0693
#171 interdiff--2809635-170-171.txt415 bytessmaz
#171 add-umami-profile-to-drupal-core--2809635-171.patch3.16 MBsmaz
#170 interdiff--2809635-169-170.txt5.35 KBsmaz
#170 add-umami-profile-to-drupal-core--2809635-170.patch3.16 MBsmaz
#169 interdiff--2809635-157-169.txt2.05 KBnavneet0693
#169 add-umami-profile-to-drupal-core--2809635-169.patch3.17 MBnavneet0693
#157 2809635-3-157.patch3.16 MBalexpott
#157 146-157-interdiff.txt5.59 KBalexpott
#146 2809635-3-145.patch3.17 MBalexpott
#146 144-145-interdiff.txt9.91 KBalexpott
#144 interdiff--2809635-139-144.txt715 bytesnavneet0693
#144 add-umami-profile-to-drupal-core--2809635-144.patch3.17 MBnavneet0693
#142 umami-142.patch3.16 MBlarowlan
#142 interdiff-604014.txt4.42 KBlarowlan
#139 interdiff--2809635-134-139.txt2.84 KBnavneet0693
#139 add-umami-profile-to-drupal-core--2809635-139.patch3.17 MBnavneet0693
#138 interdiff--2809635-134-138.txt2.28 KBnavneet0693
#138 add-umami-profile-to-drupal-core--2809635-138.patch3.17 MBnavneet0693
#137 interdiff-32597d.txt2.27 KBlarowlan
#134 add-umami-profile-to-drupal-core--2809635-134.patch3.16 MBmarkconroy
#134 interdiff--2809635-128-134.txt1.1 KBmarkconroy
#129 interdiff--2809635-127-128.txt4.79 MBmarkconroy
#129 add-umami-profile-to-drupal-core--2809635-128.patch3.16 MBmarkconroy
#127 2809635-127.patch3.16 MBalexpott
#127 125-127-interdiff.txt11.99 KBalexpott
#125 add-umami-profile-to-drupal-core--2809635-125.patch3.16 MBmarkconroy
#125 interdiff--2809635-123-125.txt4.78 MBmarkconroy
#123 add-umami-profile-to-drupal-core--2809635-123.patch3.16 MBmarkconroy
#123 interdiff--2809635-107-123.txt11.08 KBmarkconroy
#107 add-umami-profile-to-drupal-core--2809635-107.patch3.16 MBmarkconroy
#107 interdiff--2809635-105-107.txt4.6 KBmarkconroy
#105 add-umami-profile-to-drupal-core--2809635-105.patch3.16 MBmarkconroy
#103 interdiff-2809635-100-103.txt24.71 KBEli-T
#103 add-umami-profile-to-drupal-core--2809635-103.patch3.16 MBEli-T
#100 add-umami-profile-to-drupal-core--2809635-100.patch3.16 MBmarkconroy
#99 add-umami-profile-to-drupal-core--with-config-files-included--2809635-99.patch2.59 MBmarkconroy
#99 add-umami-profile-to-drupal-core--with-no-config-files--2809635-99.patch2.43 MBmarkconroy
#95 Screen Shot 2018-01-11 at 3.05.43 PM.png1.41 MBwebchick
#95 Screen Shot 2018-01-11 at 3.04.23 PM.png1.48 MBwebchick
#93 Screen Shot 2018-01-11 at 2.14.32 PM.png187.27 KBwebchick
#91 Screen Shot 2018-01-11 at 1.55.12 PM.png411.93 KBwebchick
#90 add-umami-profile-to-drupal-core--with-config-files-included--2809635-90.patch2.36 MBmarkconroy
#90 add-umami-profile-to-drupal-core--with-no-config-files--2809635-90.patch2.21 MBmarkconroy
#63 add-umami-profile-to-drupal-core--with-config-files-included--2809635-63.patch2.37 MBmarkconroy
#63 add-umami-profile-to-drupal-core--with-no-config-files--2809635-63.patch2.22 MBmarkconroy
#59 add-umami-profile-to-drupal-core--with-config-files-included--2809635-59.patch2.37 MBmarkconroy
#59 add-umami-profile-to-drupal-core--with-no-config-files--2809635-59.patch2.22 MBmarkconroy
#54 profile-warnining.png229.01 KBsmaz
#50 add-umami-profile-to-drupal-core--with-no-config-files--2809635-50.patch1.98 MBmarkconroy
#50 add-umami-profile-to-drupal-core--with-config-files-included--2809635-50.patch2.14 MBmarkconroy
#45 2809635-code-only.do-not-test.patch40.78 KBlarowlan
#40 add-umami-profile-to-drupal-core-2809635-40.patch2.14 MBmarkconroy
#35 add-umami-profile-to-drupal-core-2925065-2.patch480.4 KBmarkconroy
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

lauriii created an issue. See original summary.

davidhernandez’s picture

Is there an open issue for the "default content" initiative? We should add that as a related issue.

Gábor Hojtsy’s picture

yoroy’s picture

Discussed in yesterdays UX meeting: https://youtu.be/Lu4t2HNAOIk?t=5m33s

larowlan’s picture

tkoleary’s picture

tkoleary’s picture

larowlan’s picture

Priority: Normal » Major
Status: Active » Needs review

As per recommendation from @webchick

There is consensus from our working group on this approach

webchick’s picture

Status: Needs review » Reviewed & tested by the community

Since there's consensus from a broad-based group of people, escalating to RTBC.

catch’s picture

One question I have is what happens when a module becomes stable, does it then move out of the install profile? Also we currently have experimental modules in the work that will likely conflict until they're both stable.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Discussed this with the other core committers and members of the UX team today.

There are actually 3 "whys" wrapped into this proposal:
- Provide default sample content to demonstrate how Drupal works.
- Showcase nice-looking theme around this sample content.
- Showcase features that are not yet stable. (Nice to have.)

Some notes:
- +1 to the idea of experimental profile, especially one that integrates the experimental features.
- Concerns that there's no adequate way to warn people ahead of time the implications of Experimental Profiles in the same way we can in Modules. We've tried putting labels on experimental modules before, and it was not remotely clear enough what the ramifications of that are. For example, while experimental, the description can clearly state "This is for demonstration purposes only; not intended to be a starting point as a Drupal site."
- One thing for possible future we need to watch is if we start utilizing this pattern extensively, we could end up with "install profile creep" caused by an overwhelming number of install profiles, making the initial choice super confusing for people. But for now, the scope here is well-defined.

So in terms of the "what/why," we are on board!

In terms of the "how", a blocking release management concern is to document/mockup how we intend to make the ramifications of an experimental profile clear, and talk about how we intend to make changes in the future to the profile for existing sites. So the ensuing plan issue will need to make this clear, before we can formally approve this for inclusion in core.

So: Idea: approved! :) Next up: a plan issue that lays out the full specification, and addresses the concerns above. w00t!

larowlan’s picture

Will create a plan issue in the next 7 days (hopefully today) that starts to detail the how

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

larowlan’s picture

In light of #2759849-153: Create a new user-facing core theme this needs to be reopened, I asked in the #example-content slack channel if that comment meant the example content/profile initiative is on hold. @Gabor replied

yeah unfortunately the PM team is not on the same page re whether this is useful and how it should be useful :confused:
Gábor Hojtsy’s picture

I don't think that comment says adding an experimental install profile (with all the details summarised in this issue summary) is not a good idea?

tkoleary’s picture

Yes, as far as I understand the concept of having an experimental instal profile is settled and the open questions are around the details of the plan for theme and content.

yoroy’s picture

Component: Idea » Proposed Plan
Status: Closed (fixed) » Active

Lets keep working in here and figure out what actually implementing an experimental install profile means.

yoroy’s picture

yoroy’s picture

Issue summary: View changes
yoroy’s picture

yoroy’s picture

Issue summary: View changes
yoroy’s picture

yoroy’s picture

Issue summary: View changes
e0ipso’s picture

I think this could also be very useful for a hypothetical API-First distribution. I'm hoping we'll be able to reuse much of the work here.

#2873748: Contenta CMS: An API-first Distribution.

webchick’s picture

Tagging it up.

yoroy’s picture

I know there's work being done on this but it's quite invisible at the moment. I think it would be useful to get an update on the current approach and status in here. Thank you!

kjay’s picture

Issue summary: View changes

Work has started on this installation profile and as planned it's based on the work already completed by Contenta.

The work is currently being carried out in github where there is a composer project that will provide an installation of both the profile and the umami theme:

https://github.com/gareth-fivemile/umami-build

The profile itself is in github at:

https://github.com/gareth-fivemile/demo_umami

@smaz is currently working on getting default content to migrate into the umami demo theme when the profile is installed

markconroy’s picture

Issue summary: View changes
kjay’s picture

Another update:

The installation profile continues to be available to try out as a composer project at:

https://github.com/gareth-fivemile/umami-build

It is simple to install and is up to date with our work in progress.

Done so far:

  • Integrated the work from Contenta to provide us with sample content which includes:
  • - some articles
  • - many recipes
  • - some authors
  • Although not integrated into the profile yet, we have 6 articles complete that will be included in the coming week
  • On installation of the profile, the umami theme is installed
  • After installing the profile, install the default content module to have all content installed
  • @smaz has created a project page for the profile https://www.drupal.org/project/demo_umami This includes details on installation.
  • A view listing recipes using the card display is available on /recipes (see note below about images)
  • The recipe content type has been extensively themed and includes the custom icons produced for the theme

Being worked on right now:

  • Images are proving tricky to get working on installation of the profile. @smaz is looking into this but any advice is welcome!
  • Content is taken as-is from Contenta and the fit is not quite complete. @kjay is working on the content to improve this installation profile experience and to help with theming
  • Installation of the content by default upon installation of the profile
  • Categories, tags and the approach for setup of the further work on Views for the recipes listing page
  • Discussion on switching to the experimental field_layout module

To review the profile code, please see: https://github.com/gareth-fivemile/demo_umami
To review the umami theme code, please see: https://github.com/lauriii/umami

We are currently working on how this can exist as a patch or patches.

webchick’s picture

OMG, GREAT!! :D Thanks so much for the update!!

kjay’s picture

I forgot to add the link to our installation that rebuilds each time we commit to master (I think that's how it works ;) )

Theme work is focused on the recipes section so two example links would be:

https://master-7rqtwti-acn64pnrbyo7q.eu.platform.sh/recipes
https://master-7rqtwti-acn64pnrbyo7q.eu.platform.sh/node/10

Wonky bits courtesy of content not being quite the same as our static prototype ;) We are working on this right now.

Also worth mentioning that since a lot of theme is created under the previous styleguide work, quite a few of the styles are already done across the site, they just need wiring up as we go against the site building.

larowlan’s picture

Looking great @kjay et al - loving seeing this coming to life

e0ipso’s picture

@kjay or anyone else involved, feel free to drop in the #contenta channel in Slack if you need help leveraging some of the features we worked on. In particular I'm thinking about Installation of the content by default upon installation of the profile.

markconroy’s picture

Adding patch to add Umami profile to Drupal Core.

The patch will not apply to Drupal without the patch for the theme as well, which can be found here https://www.drupal.org/project/drupal/issues/2904243

webchick’s picture

Status: Active » Needs review

Since there's a patch, marking needs review.

larowlan’s picture

  1. +++ b/core/profiles/demo_umami/demo_umami.profile
    --- /dev/null
    +++ b/core/profiles/demo_umami/demo_umami_content/__config/install/comment.type.comment.yml
    
    +++ b/core/profiles/demo_umami/demo_umami_content/__config/install/comment.type.comment.yml
    --- /dev/null
    +++ b/core/profiles/demo_umami/demo_umami_content/__config/install/comment.type.recipe_review.yml
    
    +++ b/core/profiles/demo_umami/demo_umami_content/__config/install/comment.type.recipe_review.yml
    @@ -0,0 +1,10 @@
    diff --git a/core/profiles/demo_umami/demo_umami_content/__config/install/core.base_field_override.node.page.promote.yml b/core/profiles/demo_umami/demo_umami_content/__config/install/core.base_field_override.node.page.promote.yml
    
    --- /dev/null
    +++ b/core/profiles/demo_umami/demo_umami_content/__config/install/core.base_field_override.node.page.promote.yml
    

    What's with the __config folder?

  2. +++ b/core/profiles/demo_umami/demo_umami_content/src/CSVFileObject.php
    @@ -0,0 +1,117 @@
    + * @package Drupal\migrate_source_csv.
    ...
    +class CSVFileObject extends \SplFileObject {
    

    This looks to be copied from migrate_source_csv module?

    This probably belongs inside migrate in core so others can use it beyond this profile - and could be a separate issue/patch to go in first.

  3. +++ b/core/profiles/demo_umami/demo_umami_content/src/MigrationRunner.php
    @@ -0,0 +1,71 @@
    +class MigrationRunner {
    

    same, I see merit in this being in core generally, I've written similar classes for client projects, could go in on its own in an earlier patch

  4. +++ b/core/profiles/demo_umami/demo_umami_content/src/Plugin/migrate/source/CSV.php
    @@ -0,0 +1,156 @@
    +class CSV extends SourcePluginBase {
    

    Same, there is merit in this living in core on its own right

webchick’s picture

Project: Drupal core ideas » Drupal core
Version: » 8.5.x-dev
Component: Proposed Plan » install system

Since we're doing codey things here now, moving to the Drupal core queue.

smaz’s picture

1. What's with the __config folder?

This was leftover from when I was bringing in some of the work from Contenta - I've removed that from the github repository, so will be fixed in the next patch.

markconroy’s picture

Hi All,

Here's a patch to add the new demo_umami profile to Drupal core.

It needs some eyes to test it - don't worry a lot of it is YML config files, the actual custom code is not so big, so don't be afraid!!!

Also, for this to work, you will need the umami theme - latest patch of theme is here: https://www.drupal.org/project/drupal/issues/2904243#comment-12402906

kjay’s picture

Following on from @markconroy's patch, here's some background and notes for this work in progress patch:

This patch reworks our previous approach of using core migrations, which currently doesn't support CSV imports, by implementing self-contained custom scripts for content import upon install. We hope to return to using core migrations as soon as CSV support is available.

We are working towards release of a follow up patch later this week that covers the following points:

  1. Creation of blocks with sample content upon install and placement of these custom blocks. For example the hero banner at the top of the recipes listing page. We have a working system and this work is just being prepared for inclusion/review
  2. Renaming of the custom installation functions so that they are considered internal
  3. The tests directory has been left in this patch for now as discussion continues on the what/where/when for tests
  4. Custom content created for the demo, including images, that will replace the content items in this patch
  5. Further configuration in support of the ongoing site building and theming

A discussion is also taking place on the issue of uninstallation for the content module, but this issue is likely far reaching and we hope this will not cause issues for the alpha release

It would be great to get as much review of the profile and installation of sample content as possible. And as @markconroy says, most of this patch is config files as exported by Drupal

Status: Needs review » Needs work

The last submitted patch, 40: add-umami-profile-to-drupal-core-2809635-40.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

larowlan’s picture

@markconroy is there a chance you could upload two patches? One with the yml and one without?

Will make reviewing easier.

markconroy’s picture

Hi Larolan,

Yes, I guess I can do that. It won't be tonight though - I'll get it done as my first task tomorrow. Thanks a lot.

larowlan’s picture

Hi Mark

Don't worry, I was reviewing locally, so did it whilst at it.

Code only changes attached.

This excludes the /config and /default_content folders as well as LICENSE

Lee

larowlan’s picture

  1. +++ b/profiles/demo_umami/demo_umami.info.yml
    @@ -0,0 +1,43 @@
    +  - node
    

    I reckon we should enable big_pipe by default, just like standard does

    And ideally, media and content moderation, to show off our shiny things

  2. +++ b/profiles/demo_umami/demo_umami_content/demo_umami_content.info.yml
    @@ -0,0 +1,16 @@
    +  # - comment
    

    can be removed given the main profile doesn't need it?

  3. +++ b/profiles/demo_umami/demo_umami_content/demo_umami_content.install
    @@ -0,0 +1,256 @@
    + * Module installation file.
    

    nit: I think there is a standard format for this in an .install file

  4. +++ b/profiles/demo_umami/demo_umami_content/demo_umami_content.install
    @@ -0,0 +1,256 @@
    +      $node = \Drupal\node\Entity\Node::create($values) ;
    

    We don't need to use a fqcn here

  5. +++ b/profiles/demo_umami/demo_umami_content/demo_umami_content.install
    @@ -0,0 +1,256 @@
    +function _get_term($term, $vocabulary = 'tags') {
    ...
    +function _get_user($name) {
    ...
    +function _get_image_file($path) {
    

    Let's put these on a utility class instead of in the global namespace.

    I will have a crack at that today

  6. +++ b/profiles/demo_umami/demo_umami_content/demo_umami_content.services.yml
    @@ -0,0 +1,4 @@
    +  demo_umami_content.migration_runner:
    
    +++ b/profiles/demo_umami/demo_umami_content/src/CSVFileObject.php
    @@ -0,0 +1,117 @@
    +class CSVFileObject extends \SplFileObject {
    
    +++ b/profiles/demo_umami/demo_umami_content/src/MigrationRunner.php
    @@ -0,0 +1,71 @@
    +class MigrationRunner {
    

    no longer needed?

  7. +++ b/profiles/demo_umami/demo_umami_content/src/Plugin/Block/UmamiRecipesBannerTemp.php
    @@ -0,0 +1,36 @@
    +class UmamiRecipesBannerTemp extends BlockBase {
    

    Is this still needed? I assume this is the header stuff that is referenced in the comments as 'to do'

  8. +++ b/profiles/demo_umami/demo_umami_content/src/Plugin/migrate/source/CSV.php
    @@ -0,0 +1,156 @@
    +class CSV extends SourcePluginBase {
    
    +++ b/profiles/demo_umami/demo_umami_content/src/Plugin/migrate/source/Directory.php
    @@ -0,0 +1,79 @@
    +class Directory extends SourcePluginBase {
    
    +++ b/profiles/demo_umami/demo_umami_content/src/Plugin/migrate/source/RecipeExternalImages.php
    @@ -0,0 +1,38 @@
    +class RecipeExternalImages extends CSV {
    
    +++ b/profiles/demo_umami/demo_umami_content/src/Plugin/migrate/source/Terms.php
    @@ -0,0 +1,32 @@
    +class Terms extends CSV {
    

    not needed?

larowlan’s picture

https://github.com/gareth-fivemile/demo_umami/pull/18 covers items 2-6 and 8 from my review

I rewrote the install code to be OO and also make it more explicit what each column is, e.g. I think $data['title'] is easier to understand than $data[0]

The tests are failing, because the reinstall code is trying to install a module from the demo_umami profile, but is using the minimal install profile.

Drupal won't discover modules across profile boundaries like that.

So the best bet would be to change the test to use the demo_umami profile protected $profile = 'demo_umami'; and then rewrite the tests to assume that the module is installed already.

I'll have a crack at that and push to the same pull request.

larowlan’s picture

Pushed fix for the test to the same branch, needed to add some schemas for the blocks and move them out of the content module into the actual profile.

larowlan’s picture

Also, implemented the 'delete content on uninstall of demo module' functionality w/ test coverage

markconroy’s picture

Hi All,

Two more patches here. One of the them has the config files, one of them doesn't.

to install the profile, you will need the one with the config files (naturally!) and also the umami theme - https://www.drupal.org/project/drupal/issues/2904243

bojanz’s picture

- Concerns that there's no adequate way to warn people ahead of time the implications of Experimental Profiles in the same way we can in Modules. We've tried putting labels on experimental modules before, and it was not remotely clear enough what the ramifications of that are. For example, while experimental, the description can clearly state "This is for demonstration purposes only; not intended to be a starting point as a Drupal site."

I just saw a screenshot on Twitter that demonstrated this.
The problem is, it doesn't say what the consequences are for starting a site based on the profile. Therefore, you should expect most people to completely ignore the warning.

I am saying this from experience. When we built Commerce Kickstart v2 in early 2012, we included a real profile, and one with demo content, that had a warning saying "don't use on production", cause D7 limitations made it hard to remove some of the structure (product types & fields). Over the years we've received hundreds of angry comments about people's lives "ruined" due to having to start over. They persist to this day.

That, coupled with the experimental modules in D8 story, makes me think that either the warning needs to be a lot stronger & more detailed, or removed.

jibran’s picture

+++ b/core/profiles/demo_umami/demo_umami.install
@@ -0,0 +1,75 @@
+  \Drupal::service('module_installer')->install(['demo_umami_content'], TRUE);

+++ b/core/profiles/demo_umami/demo_umami_content/demo_umami_content.install
@@ -0,0 +1,24 @@
+  \Drupal::classResolver()->getInstanceFromDefinition(InstallHelper::class)->importArticles()
+    ->importRecipes()
+    ->importPages();

These should be wrapped in if(!\Drupal::service('config.installer')->isSyncing()){}.

ckrina’s picture

@bojanz Thank you for the feedback about the warning message, in fact Commerce Kickstart was already mentioned. People ignoring it is one of the big concerns here and one additional solution we are proposing is #2934374: Add permanent warning message for experimental profiles to avoid its usage on production sites.

For the Warning in the Installation page itself, there are some alternative design proposals already #2822414: Redesign the 'install profile selection' installer screen to allow for experimental profiles and more information and it would be great to have your input based on your direct experience, even if it's not only for the current MVP version.

smaz’s picture

FileSize
229.01 KB

What about an installation step for explaining a bit more about why experimental profiles (as this Umami demo may not be the only one in future) should only be used for testing / review purposes?

Something like the below - note that this is very quickly knocked together, but if people are interested I can turn it into a basic patch for testing.

profile warning message

yoroy’s picture

I'll copy-paste my comment from https://www.drupal.org/project/drupal/issues/2822414#comment-12405112:

I think the real point of no return is not on the installer but somewhere along the path of starting to modify, edit the Umami site. Not when choosing to install a test, but when you've changed so much that you become invested in what you have and want to keep it.

So, I don't agree that A (on the installer) is more important than B (while using, after install). Since we already know people don't really read, hanging this on A gives us only a single opportunity to get the point across, at a time when you can't even know what you are about to get/might lose. Instead, a (semi) continously nagging message/banner/notification in the site itself could be more effective because it would be visible more often.

All just to say that we can't expect people to understand what they are signing off on without having seen it. We will never fool proof this and I'm not sure if stronger warnings and extra steps would help. Yes we should warn people, but I suggest we keep whatever we do inside that single profile selection page, without any extra steps.

Gábor Hojtsy’s picture

I agree with Roy. We kept displaying various visible warnings about alpha modules and that did not work, so we should do some warning here, but making it "Are you sure that you said you are sure?" would be more distracting then useful based on previous experience.

davidhernandez’s picture

we can't expect people to understand what they are signing off on without having seen it

Also agree with Roy. People change their mind later, not at the point of install.

ok_lyndsey’s picture

Suggestion that the word Educational would be useful, and then a link to a page that explains what the purpose of the profile is with the technical reasons as to why it shouldn't be used on a production site.

Educational for demonstration purposes is a clear term. An experimental module to me means something that is likely to become stable because people are working on it; it would be a reasonable assumption to think that an experimental theme could be treated the same way.

Can we also have a summary page in the documentation about why this theme shouldn't be used in production sites? This should be pitched at managers (who may not build in Drupal) and customers. Thinking specifically of the types of issues Drupal Commerce Kickstart users face thinking they can use that. If customers/managers don't read it developers need to be able to explain clearly and in plain English why modifying the text and images isn't going to work.

markconroy’s picture

Attaching new patches for this. As always one with the config files if you want to install the profile and one without the config files if you just want to review the code.

The last submitted patch, 59: add-umami-profile-to-drupal-core--with-no-config-files--2809635-59.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 59: add-umami-profile-to-drupal-core--with-config-files-included--2809635-59.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

JayKandari’s picture

Ref: #2822414: Redesign the 'install profile selection' installer screen to allow for experimental profiles and more information

Hello,

I'm bringing up the problem of notifying users of the experimental profile. In the above-referenced thread, One option (B) was to use *hook_requirements()* and show user warning of experimental Installation profile.

This is the issue thread in which this is covered: https://github.com/gareth-fivemile/demo_umami/issues/36
Raised a PR which adds a requirement in 'runtime' phase, By default a default REQUIREMENT_WARNING is issued, if the user changes Drupal version, REQUIREMENT_ERROR is issued.

Let's look at what else can be done?
Reviews/Comments?
Thanks!

markconroy’s picture

Attaching new patches for this. As always one with the config files if you want to install the profile and one without the config files if you just want to review the code.

The sample content in almost complete in these ones, and reviews of this would be very welcome.

andrewmacpherson’s picture

Issue summary: View changes

Today at the monthly NWDUG meetup in Manchester, we took the install profile for a test drive. There's a Google doc of all the things we found, and @eli_t is preparing a PR for the Github profile repo.

Please add these names to the commit message. I've added a section for these to the issue summary too.

  • big_man
  • budda
  • eli_t
  • john-cook
  • Paul_Gregory
  • Thatdamnqa
  • philipnorton42
  • andrewmacpherson
  • nathancz

Reviewing the sample content was a really fun thing to do at a local meetup. Some of the participants don't have a core credit yet, and this was their first step in core contribution!

Google doc: https://docs.google.com/document/d/1JvRiVyx-xob6UC5ajW7DxNg9qkEg2uKhOrW3...

larowlan credited budda.

larowlan’s picture

larowlan credited big_man.

larowlan’s picture

larowlan’s picture

larowlan’s picture

larowlan’s picture

larowlan credited nathancz.

larowlan’s picture

larowlan’s picture

added those users but couldn't find john-cook or Thatdamnqa

andrewmacpherson’s picture

larowlan’s picture

larowlan’s picture

andrewmacpherson’s picture

"h2cm" needs a credit too for the NWDUG group review, omitted from the Google Doc.

larowlan credited h2cm.

larowlan’s picture

larowlan’s picture

Eli-T’s picture

As organiser, reviewer and PR author, I wouldn't mind a credit if there's one going please :)

markconroy’s picture

Just checking to see if this adds @eli-t and others to the commit credit.

yoroy’s picture

markconroy’s picture

Latest patches, one without config files for code review, one with config files for installing.

Neither patch has the licence or composer.json file since these will not be in the final version.

webchick’s picture

Ok, reviewed #90, --with-config-files-included.

The install profile selection of the installer looks like this:

Umami Demo

A few pieces of feedback here:

  • I think the short name here should just be "Demo" (or maybe "Quick demo," to further attempt to inform people not to build production websites with it). This would provide better parity with the other options, and Umami is still mentioned in the description.

    I think I remember this was done with the idea of making room for future demos, but atm we just have the one, so let's cut the terminology as much as possible, IMO.

  • (This was brought up in UX meeting earlier this week.) The big-ass YELLOW warning box below the description really draws your eye's attention to this option, so rather works against what our goal to emphasize the other options in this list as the ones that people should be choosing most of the time. However, we're working through this on #2822414: Redesign the 'install profile selection' installer screen to allow for experimental profiles and more information, so I'm ok with committing this as-is for now with the idea of cleaning it up later (before beta, if we're targeting 8.5) in a dedicated design issue.
  • (nit) I believe we use American-style language in user interface text, so the . should go inside the ""s on "Out of the Box."

I then tried to continue but get:

The website encountered an unexpected error. Please try again later.
InvalidArgumentException: Unknown themes: umami. in Drupal\Core\Extension\ThemeInstaller->install() (line 114 of core/lib/Drupal/Core/Extension/ThemeInstaller.php).

Drupal\Core\Extension\ThemeInstaller->install(Array, 1) (Line: 164)
Drupal\Core\Extension\ThemeHandler->install(Array) (Line: 1563)
install_profile_themes(Array) (Line: 671)
install_run_task(Array, Array) (Line: 549)
install_run_tasks(Array) (Line: 117)
install_drupal(Object) (Line: 44)

So looks like the theme needs to go in first, then the profile? Or both committed at the same time? Or?

At any rate, will post back again once I've got a simplytest running with both patches. :)

webchick’s picture

To save the next patch reviewer some time:

- The first 1/3 of the patch is just YML files exporting the config of things like block placement, etc. so can be skipped.
- core/profiles/demo_umami/demo_umami.profile is identical to core/profiles/standard/standard.profile (sets contact form recipient email address to match site email address), so can be skipped.
- There's also a bunch of gobbeldy gook (images) in core/profiles/demo_umami/demo_umami_content/default_content/images/ that can be skipped.
- Test coverage is in core/profiles/demo_umami/demo_umami_content/tests/src/Functional/UninstallDefaultContentTest.php and core/profiles/demo_umami/tests/src/Functional/DemoUmamiProfileTest.php.

In core/profiles/demo_umami/demo_umami_content/demo_umami_content.info.yml, we should probably add the umami theme to the list of dependencies. This would have avoided the messy error in #91.

I notice the CSV files are still there, as is the big core/profiles/demo_umami/demo_umami_content/src/InstallHelper.php class to import content. Did a framework manager sign off on this? Because last time we talked with committers, @catch (release manager) was advocating for this being a simplified, generic profile with no fanciness, that incorporated current best practices and could be copy/pasted from. (Meaning, a bunch of whatever the equivalent of node_save() is these days.) If we're going to keep the helper class + import fanciness, I'm worried that we probably need to extend the test coverage because there's quite a lot going on in that class. :\

webchick’s picture

For example, I think something's goofy with the user import code, because all of the non-admin users are showing as blocked in admin/people, which seems not on purpose?

non-admin users are blocked.

markconroy’s picture

Afaik those users are supposed to be taxonomy terms or something, not user accounts.

The idea being that a magazine could have a guest post from a celebrity for example without that person needing to have an account on the site.

webchick’s picture

Ok, continuing with the functional/visual review...

Switched the theme to Stark instead to focus on the views/content. (I can't switch it back because the theme is marked hidden, sniff!)

I notice many of the images have "Placeholder Image X" and some attribution information on them:

Picture of mushrooms from stocksnap.io

However, not all of them do:

Picture of carrots

Is that because we own the rights to the carrots image? Or is this an oversight?

And if the intent is to eventually own all the rights to all the images and thereby remove the "placeholder" text from all images to nicen up the design by quite a bit, what's the plan/timeline for this?

markconroy’s picture

speaking off top of my head here, but ... afaik we will be using only images we own (to make sure we don't have licencing issues). We won't have all the images for Monday, but will for 8.5.0

I think that is correct. Someone else might chip in with more info

webchick’s picture

Ok, running out of steam for now. Overall, I'm SUPER impressed at how this is all coming together! :D :D

Explicitly tagging "Needs framework manager review" to check into the CSV import code approach. I did talk to @larowlan about it some, and he said that he'd worked on simplifying the code that was there and removing the Migrate requirement. As long as other FMs are ok with this, that's fine with me.

lauriii’s picture

Would be great to get an explicit approval from the product management team.

markconroy’s picture

Latest patches attached.

Creates new view mode for 'highlighted medium' and also places the recipes banner block on the homepage.

markconroy’s picture

Here it is folks - our first complete patch to add the demo_umami and umami theme to Drupal core!!!

budda’s picture

The 404 page could do with a tidy up still - example https://du7gz.ply.st/articles/35643456

Same goes for the 403 access denied page - example https://du7gz.ply.st/user/1

budda’s picture

Why do we need a 'Preview' button on the 'Website feedback' form at https://du7gz.ply.st/contact -- seems odd and unexpected.

Eli-T’s picture

Installing through the GUI after applying patch in #100 leads to a url /core/install.php/recipes/super-easy-vegetarian-pasta-bake in the hero block on the homepage after installation.

This patch fixes that - note the interdiff is slightly noisier than you might expect as there were odd line endings in some places in the patch in #100.

markconroy’s picture

Issue summary: View changes
markconroy’s picture

Here's a new patch with @eli-t's work from above and also a small fix for an error we were getting when trying to edit image styles.

navneet0693’s picture

Issue summary: View changes
markconroy’s picture

Some small tweaks to fix the search results page.

smaz’s picture

Status: Needs work » Needs review

Setting as needs review! :)

The last submitted patch, 35: add-umami-profile-to-drupal-core-2925065-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 50: add-umami-profile-to-drupal-core--with-config-files-included--2809635-50.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 50: add-umami-profile-to-drupal-core--with-no-config-files--2809635-50.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 63: add-umami-profile-to-drupal-core--with-no-config-files--2809635-63.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 63: add-umami-profile-to-drupal-core--with-config-files-included--2809635-63.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 90: add-umami-profile-to-drupal-core--with-config-files-included--2809635-90.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 90: add-umami-profile-to-drupal-core--with-no-config-files--2809635-90.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 99: add-umami-profile-to-drupal-core--with-config-files-included--2809635-99.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 99: add-umami-profile-to-drupal-core--with-no-config-files--2809635-99.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 100: add-umami-profile-to-drupal-core--2809635-100.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 103: add-umami-profile-to-drupal-core--2809635-103.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 105: add-umami-profile-to-drupal-core--2809635-105.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 107: add-umami-profile-to-drupal-core--2809635-107.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

smaz’s picture

Assigned: Unassigned » smaz

Just working on some of those failures.

markconroy’s picture

This should fix the coding standards issues.

Status: Needs review » Needs work
markconroy’s picture

Moving the default content directory to a /modules directory.

navneet0693’s picture

Status: Needs work » Needs review

Setting to needs review :-)

alexpott’s picture

Here's some things from a review.

  • Some files are not the correct file mode.
  • Some of the default configuration is changing upon being installed - I've added a test for this which will also help us make sure it stays this way. Also it turns out core/profiles/demo_umami/config/optional/block.block.umami_banner_recipes.yml is being created - but with a different ID :)
  • In the out-of-thebox slack I raised concerns about adding 2.5mb of images to every Drupal site. A follow-up has been created #2936841: Remove images from demo_umami profile and download upon installation instead

Sorry for not working via github but the interdiff should be easy to apply to the repository.

markconroy’s picture

patch with Alex's Pott's items backported to our github repo

larowlan’s picture

Updated review credits to add those involved in the initiative

larowlan’s picture

Credited ok_lyndsey and kattekrab for reviews and manual testing, feedback provided in slack and on GitHub

The last submitted patch, 127: 2809635-127.patch, failed testing. View results

markconroy’s picture

A few coding standards issues with my manual backporting. Should be fixed now.

Status: Needs review » Needs work
larowlan’s picture

FileSize
2.27 KB

So @navneet0693 @pasan.gamage and I worked on the test fails.

https://github.com/gareth-fivemile/demo_umami/pull/66 and the attached interdiff should fix it.

navneet0693’s picture

Thanks @larowlan, I have uploaded a new patch which contains the changes to make test runner happy ;-)

navneet0693’s picture

Status: Needs review » Needs work
larowlan’s picture

Status: Needs work » Needs review
FileSize
4.42 KB
3.16 MB

I can't get that config test to finish locally, hoping its something in my setup.

I think the issue is a bad merge from @alexpott's interdiff being applied.

Cause I have to profile tests without it

Here's what I think it should have been, with some changes for default config that the test caught when I could get it to run

Status: Needs review » Needs work

The last submitted patch, 142: umami-142.patch, failed testing. View results

navneet0693’s picture

Fatal error: Cannot declare class Drupal\Tests\demo_umami\Functional\DemoUmamiProfileTest, because the name is already in use in /var/www/html/core/profiles/demo_umami/modules/demo_umami_content/tests/src/Functional/DemoUmamiProfileTest.php on line 14

Trying renaming test.

navneet0693’s picture

Status: Needs work » Needs review
alexpott’s picture

Fixing the config test. It should be in the profile test as it is testing the profile's config.

There seems to be the possibility of an infinite loop somewhere in \Drupal\Core\Config\ConfigManager::diff() which is a concern.

The last submitted patch, 142: umami-142.patch, failed testing. View results

timmillwood’s picture

A few little nitpicks, hope it helps your quest:

  1. +++ b/core/profiles/demo_umami/modules/demo_umami_content/src/InstallHelper.php
    @@ -0,0 +1,436 @@
    +          'uri' => 'internal:' . call_user_func(function () {
    +            $nodes = $this->entityTypeManager->getStorage('node')->loadByProperties(['title' => 'Super easy vegetarian pasta bake']);
    +            $node = reset($nodes);
    +            return $this->aliasManager->getAliasByPath('/node/' . $node->id());
    +          }),
    

    I wonder if this might be more performant if it were an entity query, rather than loading the whole node object. Also, would it be a little cleaner to put it in it's own protected method?

  2. +++ b/core/profiles/demo_umami/modules/demo_umami_content/src/InstallHelper.php
    @@ -0,0 +1,436 @@
    +   * Looks up a term by name.
    ...
    +  protected function getTerm($term_name, $vocabulary_id = 'tags') {
    

    It might be worth explaining here either in the docblock, or updating the method name, to suggest that this method will create the term if it doesn't exist.

  3. +++ b/core/profiles/demo_umami/modules/demo_umami_content/src/InstallHelper.php
    @@ -0,0 +1,436 @@
    +   * Looks up a file entity based on an image path.
    ...
    +  protected function getImage($path) {
    

    Similarly with this, it's not doing a get, it's doing a create.

waako’s picture

Tested the patch using simplytest.me (see https://duaw7.ply.st) (this is second test for good measure).
Tested patch locally (where site name is pre-populated as Umami Food Magazine) without any errors.

As anticipated on install profile selection page I got the description with a warning below:

Warning: this is a sample website, you should not use it as the basis for your website.

  • The Umami Demo profile installed without any issues.
  • There were no errors or warnings in the dblog.
  • All content, images were available.
  • No broken links that I could find.
  • Search works.
  • Tested on mobile and tablet.
ikit-claw’s picture

Tested the patch using simplytest.me and my local php7 / php7.1

  • The Umami Demo profile installed without any issues.
  • There were no errors or warnings in the dblog.
  • All content, images were available.
  • No broken links that I could find.
  • Search works.

Simpletest reported "Failed to create style directory: public://styles/square_large/public" error in dblog. This is a server configuration issue. Did not occur locally.

ikit-claw’s picture

Status: Needs review » Reviewed & tested by the community

Changed to review 3 people have looked at it now.

smaz’s picture

r.e. #149, we're planning on these (plus any other non-blocking / non breaking changes) being follow up issues to fix during alpha.

timmillwood’s picture

+++ b/core/profiles/demo_umami/modules/demo_umami_content/src/InstallHelper.php
@@ -0,0 +1,436 @@
+/**
+ * Defines a helper class for importing default content.
+ */
+class InstallHelper implements ContainerInjectionInterface {

I think it would be good to mark this as @internal, even though as @smaz points out in Slack, because the class doesn't have an interface it's kinda assumed internal anyway.

alexpott’s picture

+1 to #154 mark everything provided by the demo profile as @internal.

ikit-claw’s picture

Link for the follow-up issues Out of the box follow up for stage 2

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
5.59 KB
3.16 MB

@smaz it's best to address minor nits like #149 especially things that lead to confusion ie. getImage() not getting an image but creating a File entity. Things are harder to change once a patch lands.

Patch attached addresses #149.2 & 3 and also addresses #154. I've also removed some config from the profile that is exactly the same as the module provided config so there is no need for the profile to maintain a copy.

I understand that everyone is super keen for this to land in 8.5.0 but we should follow the review process and try to pass through all the gates.

@larowlan does the @internal help mitigate your concern about people copying \Drupal\demo_umami_content\InstallHelper?

timmillwood’s picture

Thanks @alexpott, interdiff looks great!

ikit-claw’s picture

Thanks @alexpott, I'll review again

ikit-claw’s picture

Tested the patch using simplytest.me and my local php7 / php7.1

  • The Umami Demo profile installed without any issues.
  • There were no errors or warnings in the dblog.
  • All content, images were available.
  • No broken links that I could find.
  • Search works.

My previous issue seems to have resolved itself. No errors in the dblogger now.

martin_q’s picture

I tested the patch on simplytest.me

Things I noticed / didn't expect (mostly very minor, some possibly as-intended):

  • If I edit a post and try to save it (I tried the Thai green curry, /node/11/edit ), validation fails because the author name is missing, but I hadn't changed anything in this field.
  • Having an 'author' field for your articles and recipes which isn't the same as the Drupal internal node author is slightly odd, understandable, but possibly confusing for new users that both are called the same thing
  • When that validation failed, a speech-bubble type box presented the validation error message to me ("Please fill in this field"), but it seemed to be attached to the "Manage" button in the menu bar, not the field in question.
  • Footer promo link (Umami Food Magazine -> Find out more) links to http://drupal.org rather than to /about-umami
  • Four users are created upon install, but all are blocked
  • When viewing a user (e.g. /user/5 ), the row of tabs containing 'View', 'Shortcuts', 'Edit' (most important) and 'Contact' is missing. I have to go back to the list of People and click on the Edit action button to get to it.
  • "© 2018 Terms & Conditions" reads oddly. I know this is only a demo and I probably shouldn't expect the stuff in the footer to work, but T&Cs looks like it ought to be a link (but isn't), and after the year I'd expect to see a name, even a fictional one, in a copyright statement.

Apart from that...

  • The Umami Demo profile installed without any issues (and with the warnings visible both before I installed it and then in the menu bar after install).
  • There were no errors or warnings in the dblog.
  • All content, images were available.
  • I didn't find any links that were actually broken.
  • Search works.

And as you can see I was really splitting hairs in some of the issues I identified. Overall I think it's fantastic and I saw nothing of significant concern.

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

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

larowlan’s picture

Status: Needs review » Needs work

Added review credits for @waako, @ikit-claw, @timmillwood, @martin_q

@larowlan does the @internal help mitigate your concern about people copying \Drupal\demo_umami_content\InstallHelper?

Yes - we could expand it with a comment too.

If I edit a post and try to save it (I tried the Thai green curry, /node/11/edit ), validation fails because the author name is missing, but I hadn't changed anything in this field.

Four users are created upon install, but all are blocked

I think these are both the same issue.

The import helper needs to mark these users as active - needs work for that.

larowlan’s picture

When viewing a user (e.g. /user/5 ), the row of tabs containing 'View', 'Shortcuts', 'Edit' (most important) and 'Contact' i

That will be the configuration of the local tasks block - should also be resolved

larowlan’s picture

Discussed this with @alexpott and we're comfortable removing the 'Needs framework manager review' tag.

nbz’s picture

Checking on simplytest.me:

On admin/appearance Umami is not listed as a theme.

budda’s picture

@nbz that's on purpose to avoid the demo theme being used/seen by a Drupal site administrator outside of the Demo mode.

larowlan’s picture

That is by design

navneet0693’s picture

smaz’s picture

New patch:

* Patch in #169 seemed to include something to do with adding an interdiff file
* InstallHelper now exposes a public method (importContent()) for importing all the content, and the other internal methods it calls on have been marked as internal.

smaz’s picture

Also, I've removed the hidden setting from the umami theme. Because the theme lives in the demo_umami profile, it can't be seen from other profiles (standard, minimal etc.). So by showing it, it means it will show up under appearance for the demo but no others.

andypost’s picture

larowlan’s picture

Status: Needs review » Needs work
  1. +++ b/core/profiles/demo_umami/tests/src/Functional/DemoUmamiProfileTest.php
    @@ -91,4 +91,21 @@ protected function assertDefaultConfig(StorageInterface $default_config_storage,
    +    $nodes = entity_load_multiple_by_properties('node', ['title' => 'Deep mediterranean quiche']);
    

    this is deprecated, use $this->container->get('entity_type.manager')->getStorage('node')->loadByProperties() instead

  2. +++ b/core/profiles/demo_umami/tests/src/Functional/DemoUmamiProfileTest.php
    @@ -91,4 +91,21 @@ protected function assertDefaultConfig(StorageInterface $default_config_storage,
    +    $this->drupalGet('node/' . $node->id() . '/edit');
    

    you can use $node->toUrl() here instead of the string concatenation

  3. +++ b/core/profiles/demo_umami/tests/src/Functional/DemoUmamiProfileTest.php
    @@ -91,4 +91,21 @@ protected function assertDefaultConfig(StorageInterface $default_config_storage,
    +    $webassert->pageTextContains('Recipe Deep mediterranean quiche has been updated.');
    

    Nice work!

+++ b/core/profiles/demo_umami/modules/demo_umami_content/src/InstallHelper.php
@@ -78,11 +78,21 @@
+  public function importContent() {

good idea

navneet0693’s picture

Status: Needs review » Needs work
navneet0693’s picture

Status: Needs work » Needs review
larowlan’s picture

Status: Needs review » Needs work

Review of install profile and .theme file, twig files coming up next. Not reviewing css, not my area of expertise

  1. +++ b/core/modules/system/src/Tests/Installer/SingleVisibleProfileTest.php
    @@ -21,25 +21,20 @@ class SingleVisibleProfileTest extends InstallerTestBase {
    +    $profiles = ['standard', 'demo_umami'];
    +    foreach ($profiles as $profile) {
    

    Can we get a follow-up to flip this, and auto-apply hidden to all but the minimal profile, so we don't have to add new profiles here every time we add a new one?

  2. +++ b/core/profiles/demo_umami/config/optional/responsive_image.styles.wide.yml
    --- /dev/null
    +++ b/core/profiles/demo_umami/config/schema/demo_umami_content.schema.yml
    
    +++ b/core/profiles/demo_umami/config/schema/demo_umami_content.schema.yml
    +++ b/core/profiles/demo_umami/config/schema/demo_umami_content.schema.yml
    @@ -0,0 +1,31 @@
    
    @@ -0,0 +1,31 @@
    +block.settings.umami_disclaimer:
    

    either the file name is wrong, or this is in the wrong folder

  3. +++ b/core/profiles/demo_umami/modules/demo_umami_content/default_content/LICENCE.txt
    @@ -0,0 +1,23 @@
    +Creative Commons Attribution-ShareAlike 4.0 International License
    ...
    +are all licensed under a http://creativecommons.org/licenses/by-sa/4.0/ Creative
    +Commons Attribution-ShareAlike 4.0 International License.
    

    is this compatible with GPL2+ - only GPL2+ licensed content/code can be on drupal.org

  4. +++ b/core/profiles/demo_umami/src/Plugin/Block/UmamiDisclaimer.php
    @@ -0,0 +1,88 @@
    +class UmamiDisclaimer extends BlockBase {
    
    +++ b/core/profiles/demo_umami/src/Plugin/Block/UmamiFooterPromo.php
    @@ -0,0 +1,104 @@
    +class UmamiFooterPromo extends BlockBase {
    

    The blocks are defined in the demo_umami module, but the schema is named demo_umami_content.schema.yml, I think it should be named demo_umami.schema.yml

  5. +++ b/core/profiles/demo_umami/themes/umami/umami.theme
    @@ -0,0 +1,83 @@
    +  $request = \Drupal::request();
    +  if ($route = $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT)) {
    

    We should use \Drupal::routeMatch() here instead

  6. +++ b/core/profiles/demo_umami/themes/umami/umami.theme
    @@ -0,0 +1,83 @@
    +    if (($entity = $request->attributes->get('entity')) && $entity->getEntityType()->id() == 'search_page') {
    

    nit, === 'search_page', we're comparing strings here, let's be strict

    also we can use ->getEntityTypeId() instead of ->getEntityType()->id()

  7. +++ b/core/profiles/demo_umami/themes/umami/umami.theme
    @@ -0,0 +1,83 @@
    +  // Since we are printing the 'Current Page Title', set the cache to match
    +  // the URL cache. If we don't, then we might end up with something like
    +  // "Home > Articles" on the Recipes page, which should read "Home > Recipes".
    

    the english here is slightly off. Suggest 'add the url cache context' instead of 'set the cache to match the URL cache'

larowlan’s picture

Review of twig files in the theme, some of this can be follow-ups at discretion of front-end framework managers

  1. +++ b/core/profiles/demo_umami/themes/umami/js/components/navigation/menu-main/menu-main.es6.js
    @@ -0,0 +1,17 @@
    + * This file is used to add any javascrip that is needed for the main menu.
    

    missing t

  2. +++ b/core/profiles/demo_umami/themes/umami/templates/components/banner-block/block--bundle--banner-block.html.twig
    @@ -0,0 +1,54 @@
    +        {{ content.field_content_link }}
    

    I think you need to output {{content}} here too to ensure bubbling of cache tags, see https://www.previousnext.com.au/blog/ensuring-drupal-8-block-cache-tags-...

  3. +++ b/core/profiles/demo_umami/themes/umami/templates/components/search/input--search.html.twig
    @@ -0,0 +1,18 @@
    +  set placeholder_text = 'Search by keyword, ingredient, dish'
    +%}
    +
    +<input{{ attributes.setAttribute('placeholder', placeholder_text) }} />{{ children }}
    

    Are we sure doing this globally for the search element is the best option?

    Suggest a form_alter in the theme instead and then dropping this template?

  4. +++ b/core/profiles/demo_umami/themes/umami/templates/content/node--highlighted-bottom.html.twig
    @@ -0,0 +1,107 @@
    +      <a class="read-more__link" href="{{ url }}">{{ 'View'|t }} {{ node.bundle }}</a>
    

    this needs a11y context, 'View Recipe' or similar isn't specific enough for screen reader users

  5. +++ b/core/profiles/demo_umami/themes/umami/templates/content/node--highlighted-medium.html.twig
    @@ -0,0 +1,105 @@
    +    <a class="read-more__link" href="{{ url }}">{{ 'View'|t }} {{ node.bundle }}</a>
    
    +++ b/core/profiles/demo_umami/themes/umami/templates/content/node--highlighted-small.html.twig
    @@ -0,0 +1,105 @@
    +    <a class="read-more__link" href="{{ url }}">{{ 'View'|t }} {{ node.bundle }}</a>
    
    +++ b/core/profiles/demo_umami/themes/umami/templates/content/node--highlighted-top.html.twig
    @@ -0,0 +1,107 @@
    +      <a class="read-more__link" href="{{ url }}">{{ 'View'|t }} {{ node.bundle }}</a>
    

    Repeated a few times

  6. +++ b/core/profiles/demo_umami/themes/umami/templates/layout/page.html.twig
    @@ -0,0 +1,132 @@
    +  {% if page.header|render|striptags|trim is not empty %}
    ...
    +  {% if page.tabs|render|striptags|trim is not empty %}
    ...
    +  {% if page.banner_top|render|striptags|trim is not empty %}
    ...
    +  {% if page.breadcrumbs|render|striptags|trim is not empty %}
    ...
    +    {% if page.page_title|render|striptags|trim is not empty %}
    ...
    +    {% if page.sidebar|render|striptags|trim is not empty %}
    ...
    +  {% if page.footer|render|striptags|trim is not empty %}
    ...
    +  {% if page.bottom|render|striptags|trim is not empty %}
    

    Is this the only way we can do this? Surely we can do something smarter in preprocessing?

navneet0693’s picture

@larowlan Skipping We should use \Drupal::routeMatch() here, as

umami.them, Line #76, $variables['current_page_title'] = \Drupal::service('title_resolver')->getTitle($request, $route); requires second argument to an instance of Symfony\Component\Routing\Route

navneet0693’s picture

Status: Needs review » Reviewed & tested by the community

Rest issues aren't blockers for getting this committed, and follow ups are created by @markconroy. So, it's good to go I guess ;-)

larowlan’s picture

Would still like to see #177.5 fixes, \Drupal::routeMatch()->getRouteObject() should suffice instead of dealing with the magic request attributes.

Other than that, I'm +1 on RTBC.

There is a dedicated theme issue where reviews of the CSS have been occurring.

markconroy’s picture

Can we also add

thamas
mariohernandez
sharjay
lauriii
tomphippen

on the commit log.

markconroy’s picture

Issue summary: View changes
markconroy’s picture

Issue summary: View changes
markconroy’s picture

Issue summary: View changes
markconroy’s picture

Issue summary: View changes

larowlan credited thamas.

larowlan’s picture

larowlan’s picture

larowlan’s picture

larowlan’s picture

webchick’s picture

Ok, @markconroy gave an EPIC! demo of the current patch to the Drupal Product Management Team on today's meeting. Full video at https://youtu.be/BgZV8KjrOMQ for those who want to follow along. (It's a little over an hour.)

TL;DR: No commit-blocking concerns from the Product Management Team for committing this to the 8.6.x branch, so removing the tag. WOOHOO! To ship in a tagged release, however (e.g. 8.6.0), there's a bit more massaging to be done on the before/after installation warnings.

I’ll work in the next little bit on spelling this all out in a comment w/ screenshots + text for those who don’t have time to watch the video, but in the meantime hopefully this’ll do to get this unblocked. :)

GO TEAM GO!!!!

yoroy’s picture

Yep, this is good to go for initial commit. To get to something shippable we'll need followups for:

- showing the warning on the install profile selection screen only on selecting the Umami profile. So initially hidden.
- Probably tone down the visual contrast in the permanent
- There seemed to be a bug where in the small breakpoint on the homepage the lead image got shown twice
- Finetune the wording of the messages in installer, toolbar and on the status report page

yoroy’s picture

webchick’s picture

Ok, @markconroy gave an EPIC! demo of the current patch to the Drupal Product Management Team on today's meeting. Full video at https://youtu.be/BgZV8KjrOMQ for those who want to follow along. (It's a little over an hour.)

TL;DR: No commit-blocking concerns from the Product Management Team for committing this to the 8.6.x branch, so removing the tag. WOOHOO! To ship in a tagged release, however (e.g. 8.6.0), there's a bit more massaging to be done on the before/after installation warnings.

I’ll work in the next little bit on spelling this all out in a comment w/ screenshots + text for those who don’t have time to watch the video, but in the meantime hopefully this’ll do to get this unblocked. :)

Walkthrough

Here’s what the profile/theme look like in their current state. Sorry for the somewhat grainy screenshots; they’re taken from the video).

Home page (logged out)

(3:54)

Top of Umami home page showing a banner image
Middle of Umami home page showing featured content
Bottom of Umami home page showing footer

Articles

(7:21)

Articles overview page showing 4 recipes in a grid.
Single article page with sidebar showing other featured content.

Recipes

(6:20)

Single article page with sidebar showing other featured content.
Single recipe page with prep time, cooking time, and so on.
Single recipe page, preparation instructions.

No concerns raised. This all looks great. Note that it currently uses hard-coding in the templates to achieve the layout ability but Layout Builder isn’t yet a stable feature, so this makes sense for now.

The images are a little less “splashy” than the stock photos we were using previously. However, these ones we own all the rights to. ALSO. I learned that Sharon (@kjay’s wife) literally made all of the recipes so that pictures could be taken of them! :O :O That is some serious dedication!! Thank you SO much!!

Mobile

(28:14)

“Mobile
“Mobile
“Mobile

So cool to see that the mobile experience has been well taken care of!

Bug: On a couple of pages (Home and Recipes?), the header image was showing twice on mobile.

Warnings

(28:27)

A release manager requirement was to ensure that users are adequately warned about the implications of installing an experimental profile, and especially this one, which is intended as a quick throwaway demo.

Here’s the installer warning:

“Profile

Currently, what's there represents a visual regression from the existing install page, in that a) the yellow warning attracts ALL the attention, away from the things you should *usually* be clicking on here, and b) it also isn't quite clear from the visual hierarchy that it's only referring to the last profile in question and not all of them. :\

Recommendation was to only show the yellow warning upon clicking the Umami demo radio, and hide it again when clicking either of the others, using the #states system. Also to get rid of the text "Warning:" since, you know, it's already a warning. :D There will probably be further text tweaking we'll need to do, but that can be done anywhere up until RC in a tagged release so isn't the most pressing them atm. Let's discuss it on a future UX meeting.

TL;DR for now, this message is seen as good enough for initial commit to 8.6.x, but not for shipping in a tagged release. Let's continue refining in #2822414: Redesign the 'install profile selection' installer screen to allow for experimental profiles and more information.

Then after installation, we warn in two ways. The first is in the admin status page:

“Warning

This is fine to do; however, as you can see, it’s very easy to miss this in amongst the other warnings that might be on this page.

So as yet another way of informing people what Umami is for, we also add this message to the Toolbar for admin users:

“This

This warning message links to #2829101: Make it possible to add experimental themes into core, which seems fine for initial commit, but we’ll want to clean up before shipping. Even has a mobile-friendly version, which is just the icon. NICE!

@yoroy also expressed that we may want to really tone this warning down. NOT something to block commit, but something to explore more in #2934374: Add permanent warning message for experimental profiles to avoid its usage on production sites.

So, in summary.

No commit blockers with the patch.

There were a handful of “beta blockers” (blockers to being shipped in a tagged release), outlined by @yoroy in #195.

So really, my only hesitation around committing this to 8.6.x right freaking now is we need to know Sharon Jay's Drupal.org username (or create one!) so we can credit her, because no one does a week's worth of cooking for Drupal without commit credit! :D (And then maybe separately we should also buy her a spa day, but one thing at a time. ;))

yoroy credited SharJay.

yoroy’s picture

larowlan’s picture

Can we get a follow-up for #181 @navneet0693

webchick’s picture

Found her! :D Thanks, @yoroy!

Working on synchronizing the credit list in the issue summary with the credit system.

webchick’s picture

webchick’s picture

webchick’s picture

webchick’s picture

webchick credited kreynen.

webchick’s picture

webchick’s picture

larowlan’s picture

I don't think #171.4 has been addressed yet either, its just a filename though, so can be follow up too - @navneet0693 can you handle?

larowlan’s picture

Issue tags: +Needs followup

Adding needs follow-up tag for

  • webchick committed aead8ca on 8.6.x
    Issue #2809635 by markconroy, navneet0693, alexpott, smaz, larowlan, Eli...
webchick’s picture

BAM!!!!!

http://cgit.drupalcode.org/drupal/commit/?h=8.6.x&id=aead8ca4089e716c70c...

GREAT work everyone! This has now been committed to 8.6.x. :D Hopefully subsequent changes are now a LOT easier to review/commit since they're not tied to 3MB of images. ;)

Let's get those follow-ups filed and get it ready for release!!

navneet0693’s picture

DuaelFr’s picture

Well done mates :)

Wim Leers’s picture

🎉🎂🥂

Status: Reviewed & tested by the community » Needs work
phenaproxima’s picture

I learned that Sharon (@kjay’s wife) literally made all of the recipes so that pictures could be taken of them!

Holy moly! That is absolutely unreal. Somebody make this woman a cake in the shape of a Druplicon.

Amazing work here. Thanks, everyone, for your dedication. This is a huge win for Drupal.

navneet0693’s picture

Status: Needs work » Reviewed & tested by the community
markconroy’s picture

Status: Reviewed & tested by the community » Fixed

Marking as fixed.

This has been committed to 8.6.x

Please create follow up issues for anything else instead of commenting here.

markconroy’s picture

Issue tags: -Needs followup
David_Rothstein’s picture

This is really fantastic work!

In reviewing the code, I found a few big issues, which I am linking to here. I posted patches where possible (but the last one is a little more overarching and maybe needs to be broken up):
#2938904: "Find out more" link isn't validated or properly displayed, leading to bugs including cross-site scripting
#2938900: Replace profile-defined blocks with custom blocks to fix a variety of problems
#2938911: Umami theme hardcodes too many assumptions about the install profile and site configuration

David_Rothstein’s picture

Also, I've removed the hidden setting from the umami theme. Because the theme lives in the demo_umami profile, it can't be seen from other profiles (standard, minimal etc.). So by showing it, it means it will show up under appearance for the demo but no others.

Somehow that change never made it into the committed patch here, so the associated bug still exists. I created #2938918: Impossible to switch back to the Umami theme after switching away from it to fix it.

David_Rothstein’s picture

Status: Fixed » Needs review

There is actually still a draft (unpublished) change record here - https://www.drupal.org/node/2937365

So this issue should remain open until that change record is ready to publish (or alternatively, it could be associated with a different issue so that it doesn't get lost).

navneet0693’s picture

Status: Needs review » Fixed

@David thanks you for pointing out, change record is now published.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.