Working issues
- #2822414: Redesign the 'install profile selection' installer screen to allow for experimental profiles and more information
- There's a sandbox with prototype code and more tasks.
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
Comment | File | Size | Author |
---|---|---|---|
#197 | Screen Shot 2018-01-18 at 3.17.08 PM.png | 205.04 KB | webchick |
#197 | Screen Shot 2018-01-18 at 3.14.07 PM.png | 292.8 KB | webchick |
#197 | Screen Shot 2018-01-18 at 3.13.29 PM.png | 166.77 KB | webchick |
#179 | add-umami-profile-to-drupal-core--2809635-179.patch | 3.17 MB | navneet0693 |
#179 | interdiff--2809635-174-179.txt | 2.75 KB | navneet0693 |
Comments
Comment #2
davidhernandezIs there an open issue for the "default content" initiative? We should add that as a related issue.
Comment #3
Gábor HojtsyComment #4
yoroy CreditAttribution: yoroy at Roy Scholten commentedDiscussed in yesterdays UX meeting: https://youtu.be/Lu4t2HNAOIk?t=5m33s
Comment #5
larowlanAdding #2818085: Promote and allow downloading of vetted install profiles from the installer to related
Comment #6
larowlansandbox is https://www.drupal.org/sandbox/larowlan/2808575
Comment #7
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #8
tkoleary CreditAttribution: tkoleary at Acquia commentedComment #9
larowlanAs per recommendation from @webchick
There is consensus from our working group on this approach
Comment #10
webchickSince there's consensus from a broad-based group of people, escalating to RTBC.
Comment #11
catchOne 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.
Comment #12
webchickDiscussed 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!
Comment #13
larowlanWill create a plan issue in the next 7 days (hopefully today) that starts to detail the how
Comment #15
larowlanIn light of #2759849-153: Create a new user-facing core theme as part of a Drupal demo 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
Comment #16
Gábor HojtsyI 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?
Comment #17
tkoleary CreditAttribution: tkoleary at Acquia commentedYes, 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.
Comment #18
yoroy CreditAttribution: yoroy at Roy Scholten commentedLets keep working in here and figure out what actually implementing an experimental install profile means.
Comment #19
yoroy CreditAttribution: yoroy at Roy Scholten commentedComment #20
yoroy CreditAttribution: yoroy at Roy Scholten commentedComment #21
yoroy CreditAttribution: yoroy at Roy Scholten commentedComment #22
yoroy CreditAttribution: yoroy at Roy Scholten commentedComment #23
yoroy CreditAttribution: yoroy at Roy Scholten commentedComment #24
yoroy CreditAttribution: yoroy at Roy Scholten commentedComment #25
e0ipsoI 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.
Comment #26
webchickTagging it up.
Comment #27
yoroy CreditAttribution: yoroy at Roy Scholten commentedI 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!
Comment #28
kjay CreditAttribution: kjay commentedWork 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
Comment #29
markconroy CreditAttribution: markconroy commentedComment #30
kjay CreditAttribution: kjay commentedAnother 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:
Being worked on right now:
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.
Comment #31
webchickOMG, GREAT!! :D Thanks so much for the update!!
Comment #32
kjay CreditAttribution: kjay commentedI 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.
Comment #33
larowlanLooking great @kjay et al - loving seeing this coming to life
Comment #34
e0ipso@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.
Comment #35
markconroy CreditAttribution: markconroy commentedAdding 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
Comment #36
webchickSince there's a patch, marking needs review.
Comment #37
larowlanWhat's with the __config folder?
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.
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
Same, there is merit in this living in core on its own right
Comment #38
webchickSince we're doing codey things here now, moving to the Drupal core queue.
Comment #39
smazThis 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.
Comment #40
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedHi 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
Comment #41
kjay CreditAttribution: kjay commentedFollowing 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:
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
Comment #43
larowlan@markconroy is there a chance you could upload two patches? One with the yml and one without?
Will make reviewing easier.
Comment #44
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedHi 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.
Comment #45
larowlanHi 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
Comment #46
larowlanI reckon we should enable big_pipe by default, just like standard does
And ideally, media and content moderation, to show off our shiny things
can be removed given the main profile doesn't need it?
nit: I think there is a standard format for this in an .install file
We don't need to use a fqcn here
Let's put these on a utility class instead of in the global namespace.
I will have a crack at that today
no longer needed?
Is this still needed? I assume this is the header stuff that is referenced in the comments as 'to do'
not needed?
Comment #47
larowlanhttps://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.
Comment #48
larowlanPushed 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.
Comment #49
larowlanAlso, implemented the 'delete content on uninstall of demo module' functionality w/ test coverage
Comment #50
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedHi 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
Comment #51
bojanz CreditAttribution: bojanz at Centarro commentedI 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.
Comment #52
jibranThese should be wrapped in
if(!\Drupal::service('config.installer')->isSyncing()){}
.Comment #53
ckrina@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.
Comment #54
smazWhat 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.
Comment #55
yoroy CreditAttribution: yoroy at Roy Scholten commentedI'll copy-paste my comment from https://www.drupal.org/project/drupal/issues/2822414#comment-12405112:
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.
Comment #56
Gábor HojtsyI 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.
Comment #57
davidhernandezAlso agree with Roy. People change their mind later, not at the point of install.
Comment #58
ok_lyndsey CreditAttribution: ok_lyndsey as a volunteer commentedSuggestion 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.
Comment #59
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedAttaching 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.
Comment #62
JayKandariRef: #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!
Comment #63
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedAttaching 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.
Comment #64
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedToday 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.
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...
Comment #66
larowlanComment #68
larowlanComment #69
larowlanComment #71
larowlanComment #73
larowlanComment #75
larowlanComment #76
larowlanadded those users but couldn't find john-cook or Thatdamnqa
Comment #77
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commented@larowlan
"John Cook" - https://www.drupal.org/u/john-cook
"thatdamnqa" - https://www.drupal.org/u/thatdamnqa
The Github PR from the NWDUG meeting is https://github.com/gareth-fivemile/demo_umami/pull/41
Comment #79
larowlanComment #81
larowlanComment #82
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commented"h2cm" needs a credit too for the NWDUG group review, omitted from the Google Doc.
Comment #84
larowlanComment #86
larowlanComment #87
Eli-TAs organiser, reviewer and PR author, I wouldn't mind a credit if there's one going please :)
Comment #88
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedJust checking to see if this adds @eli-t and others to the commit credit.Comment #89
yoroy CreditAttribution: yoroy at Roy Scholten commentedComment #90
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedLatest 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.
Comment #91
webchickOk, reviewed #90, --with-config-files-included.
The install profile selection of the installer looks like this:
A few pieces of feedback here:
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.
I then tried to continue but get:
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. :)
Comment #92
webchickTo 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. :\Comment #93
webchickFor 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?
Comment #94
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedAfaik 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.
Comment #95
webchickOk, 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:
However, not all of them do:
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?
Comment #96
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedspeaking 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
Comment #97
webchickOk, 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.
Comment #98
lauriiiWould be great to get an explicit approval from the product management team.
Comment #99
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedLatest patches attached.
Creates new view mode for 'highlighted medium' and also places the recipes banner block on the homepage.
Comment #100
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedHere it is folks - our first complete patch to add the demo_umami and umami theme to Drupal core!!!
Comment #101
buddaThe 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
Comment #102
buddaWhy do we need a 'Preview' button on the 'Website feedback' form at https://du7gz.ply.st/contact -- seems odd and unexpected.
Comment #103
Eli-TInstalling 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.
Comment #104
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedComment #105
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedHere'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.
Comment #106
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedComment #107
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedSome small tweaks to fix the search results page.
Comment #108
smazSetting as needs review! :)
Comment #122
smazJust working on some of those failures.
Comment #123
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedThis should fix the coding standards issues.
Comment #125
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedMoving the default content directory to a /modules directory.
Comment #126
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedSetting to needs review :-)
Comment #127
alexpottHere's some things from a review.
Sorry for not working via github but the interdiff should be easy to apply to the repository.
Comment #129
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedpatch with Alex's Pott's items backported to our github repo
Comment #131
larowlanUpdated review credits to add those involved in the initiative
Comment #132
larowlanCredited ok_lyndsey and kattekrab for reviews and manual testing, feedback provided in slack and on GitHub
Comment #134
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedA few coding standards issues with my manual backporting. Should be fixed now.
Comment #137
larowlanSo @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.
Comment #138
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedThanks @larowlan, I have uploaded a new patch which contains the changes to make test runner happy ;-)
Comment #139
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedSorry, missed to include changes in PR #66.
Comment #142
larowlanI 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
Comment #144
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedTrying renaming test.
Comment #145
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedComment #146
alexpottFixing 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.Comment #149
timmillwoodA few little nitpicks, hope it helps your quest:
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?
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.
Similarly with this, it's not doing a get, it's doing a create.
Comment #150
waako CreditAttribution: waako at Annertech commentedTested 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:
Comment #151
ikit-claw CreditAttribution: ikit-claw as a volunteer commentedTested the patch using simplytest.me and my local php7 / php7.1
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.
Comment #152
ikit-claw CreditAttribution: ikit-claw as a volunteer commentedChanged to review 3 people have looked at it now.
Comment #153
smazr.e. #149, we're planning on these (plus any other non-blocking / non breaking changes) being follow up issues to fix during alpha.
Comment #154
timmillwoodI 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.Comment #155
alexpott+1 to #154 mark everything provided by the demo profile as @internal.
Comment #156
ikit-claw CreditAttribution: ikit-claw as a volunteer commentedLink for the follow-up issues Out of the box follow up for stage 2
Comment #157
alexpott@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?Comment #158
timmillwoodThanks @alexpott, interdiff looks great!
Comment #159
ikit-claw CreditAttribution: ikit-claw as a volunteer commentedThanks @alexpott, I'll review again
Comment #160
ikit-claw CreditAttribution: ikit-claw as a volunteer commentedTested the patch using simplytest.me and my local php7 / php7.1
My previous issue seems to have resolved itself. No errors in the dblogger now.
Comment #161
martin_qI tested the patch on simplytest.me
Things I noticed / didn't expect (mostly very minor, some possibly as-intended):
/node/11/edit
), validation fails because the author name is missing, but I hadn't changed anything in this field.http://drupal.org
rather than to/about-umami
/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.Apart from that...
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.
Comment #163
larowlanAdded review credits for @waako, @ikit-claw, @timmillwood, @martin_q
Yes - we could expand it with a comment too.
I think these are both the same issue.
The import helper needs to mark these users as active - needs work for that.
Comment #164
larowlanThat will be the configuration of the local tasks block - should also be resolved
Comment #165
larowlanDiscussed this with @alexpott and we're comfortable removing the 'Needs framework manager review' tag.
Comment #166
NaheemSays CreditAttribution: NaheemSays commentedChecking on simplytest.me:
On admin/appearance Umami is not listed as a theme.
Comment #167
budda@nbz that's on purpose to avoid the demo theme being used/seen by a Drupal site administrator outside of the Demo mode.
Comment #168
larowlanThat is by design
Comment #169
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedComment #170
smazNew 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.
Comment #171
smazAlso, 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.
Comment #172
andypostContact link could be missing becuse of #1191464: Inconsistent behaviour in personal contact form for users created before versus after contact module is enabled
Comment #173
larowlanthis is deprecated, use
$this->container->get('entity_type.manager')->getStorage('node')->loadByProperties()
insteadyou can use
$node->toUrl()
here instead of the string concatenationNice work!
good idea
Comment #174
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedComment #176
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedComment #177
larowlanReview of install profile and .theme file, twig files coming up next. Not reviewing css, not my area of expertise
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?
either the file name is wrong, or this is in the wrong folder
is this compatible with GPL2+ - only GPL2+ licensed content/code can be on drupal.org
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
We should use \Drupal::routeMatch() here instead
nit, === 'search_page', we're comparing strings here, let's be strict
also we can use ->getEntityTypeId() instead of ->getEntityType()->id()
the english here is slightly off. Suggest 'add the url cache context' instead of 'set the cache to match the URL cache'
Comment #178
larowlanReview of twig files in the theme, some of this can be follow-ups at discretion of front-end framework managers
missing t
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-...
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?
this needs a11y context, 'View Recipe' or similar isn't specific enough for screen reader users
Repeated a few times
Is this the only way we can do this? Surely we can do something smarter in preprocessing?
Comment #179
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commented@larowlan Skipping We should use
\Drupal::routeMatch()
here, asumami.them, Line #76,
$variables['current_page_title'] = \Drupal::service('title_resolver')->getTitle($request, $route);
requires second argument to an instance ofSymfony\Component\Routing\Route
Comment #180
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedRest issues aren't blockers for getting this committed, and follow ups are created by @markconroy. So, it's good to go I guess ;-)
Comment #181
larowlanWould 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.
Comment #182
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedCan we also add
thamas
mariohernandez
sharjay
lauriii
tomphippen
on the commit log.
Comment #183
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedComment #184
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedComment #185
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedComment #186
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedComment #188
larowlanComment #190
larowlanComment #191
larowlanComment #193
larowlanComment #194
webchickOk, @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!!!!
Comment #195
yoroy CreditAttribution: yoroy at Roy Scholten commentedYep, 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
Comment #196
yoroy CreditAttribution: yoroy at Roy Scholten commentedComment #197
webchickOk, @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)
Articles
(7:21)
Recipes
(6:20)
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)
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:
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:
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 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. ;))
Comment #199
yoroy CreditAttribution: yoroy at Roy Scholten commentedComment #200
larowlanCan we get a follow-up for #181 @navneet0693
Comment #202
webchickFound her! :D Thanks, @yoroy!
Working on synchronizing the credit list in the issue summary with the credit system.
Comment #204
webchickComment #205
webchickComment #207
webchickComment #209
webchickComment #211
webchickComment #213
webchickComment #214
larowlanI 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?
Comment #215
larowlanAdding needs follow-up tag for
Comment #217
webchickBAM!!!!!
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!!
Comment #218
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commented@larowlan Added a follow up task for : https://www.drupal.org/project/drupal/issues/2937894, https://www.drupal.org/project/drupal/issues/2937898
Comment #219
DuaelFrWell done mates :)
Comment #220
Wim Leers🎉🎂🥂
Comment #222
phenaproximaHoly 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.
Comment #223
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedComment #224
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedMarking as fixed.
This has been committed to 8.6.x
Please create follow up issues for anything else instead of commenting here.
Comment #225
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedComment #226
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThis 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
Comment #227
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedSomehow 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.
Comment #228
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThere 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).
Comment #229
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commented@David thanks you for pointing out, change record is now published.