Extend site information page to generate a manifest.json file

These all feel like things for which I'd use metatags module. Are we sure this might not be a better fit in contrib?

So the idea is to generate some route which serves this file?

We can generate a real file and stick that in public://, it'll end up being used as much as robots.txt and favicon.ico no need to make it expensive, it doesn't need to be dynamic.

Good point, as we can specify the path, there is no reason to not just generate it.

Re-roll of #4, also using short array syntax.

Reroll looks good, thanks @erik.erskine.

I believe this requires an upgrade path since its setting new configuration.

Hi there,

Nice to see this! I have added some fields in the manifest form:

• name (textfield)
• short name (textfield)
• icon (managed_file)

While the 'short_name' takes care of the name on mobile devices, the 'name' is the longer (site)name that will be used when you pin it on your homescreen in desktop browsers (e.g. chrome://apps/). The 'name' field will be automatically filled with the 'site_name' unless you change the value yourself.

I've also added a icon field. Probably the nicest thing of the manifest since it will allow you to provide that 'app-like' feel when you pin to your homescreen. However, this topic could use some discussion. It's ideal to have at least the following resolution icons:

• 128x128
• 144x144
• 152x152
• 192x192

So the big question is, how should we deal with those different icon resolutions to provide a valid icon for the most mobile devices? Here's my take on it along with the question marks I have:

1. We could take the logo from the theme as a default. But It's kinda unrealistic/hard to provide a generic way of resizing logo's from (custom)themes since they vary a lot in resolution and dimensions.
2. We could choose to have one dedicated upload field where there's at least one square icon needed with a minimum resolution of 256x256 that will be resized (with image styles?) to fit the correct resolutions.

Thanks for the patch, it was corrupt probably because a line was removed.
Re-applied your changes, and uploaded a working patch below.

This should have less errors. Not sure what the proper schema is for a sequence of files, though.

No idea why I can't run the tests locally (phpunit no longer throws the schema errors but marks all tests as skipped). Locally these tests threw in the previous version, and are now marked skipped, unlike the CI bot.

FWIW these tests now pass locally on 8.5.x HEAD using this command line:

sudo -u _www php core/scripts/run-tests.sh --verbose --sqlite /tmp/test.sqlite --url http://drop8.localhost Ajax

(to run the Ajax group which is the first failing here).

Seems to be a problem with the CI use of the --concurrency flag. When I run these tests with concurrency = 8, I get this:

Test summary
------------

Drupal\FunctionalJavascriptTests\Ajax\AjaxFormImageButtonTes   0 passes             1 exceptions
FATAL Drupal\FunctionalJavascriptTests\Ajax\AjaxFormImageButtonTest: test runner returned a non-zero error code (2).
Drupal\FunctionalJavascriptTests\Ajax\AjaxFormImageButtonTes   0 passes   1 fails
Drupal\system\Tests\Ajax\AjaxInGroupTest                      19 passes                            5 messages
Drupal\system\Tests\Ajax\DialogTest                           65 passes   2 fails   1 exceptions  24 messages
- Found database prefix 'test25569037' for test ID 14.
Drupal\FunctionalJavascriptTests\Ajax\AjaxCallbacksTest        0 passes             1 exceptions
FATAL Drupal\FunctionalJavascriptTests\Ajax\AjaxCallbacksTest: test runner returned a non-zero error code (2).
Drupal\FunctionalJavascriptTests\Ajax\AjaxCallbacksTest        0 passes   1 fails
Drupal\FunctionalJavascriptTests\Ajax\AjaxTest                 0 passes             1 exceptions
FATAL Drupal\FunctionalJavascriptTests\Ajax\AjaxTest: test runner returned a non-zero error code (2).
Drupal\FunctionalJavascriptTests\Ajax\AjaxTest                 0 passes   1 fails
Drupal\Tests\Core\Ajax\AjaxCommandsTest                       24 passes
Drupal\Tests\Core\Ajax\AjaxResponseTest                        2 passes
Drupal\system\Tests\Ajax\ElementValidationTest                15 passes                            7 messages
Drupal\FunctionalJavascriptTests\Ajax\AjaxFormPageCacheTest    2 passes
Drupal\Tests\Core\Controller\AjaxRendererTest                  1 passes
Drupal\system\Tests\Ajax\CommandsTest                         24 passes             4 exceptions   9 messages
- Found database prefix 'test90584407' for test ID 13.
Drupal\system\Tests\Ajax\FormValuesTest                       47 passes                           26 messages
Drupal\system\Tests\Ajax\MultiFormTest                        42 passes                           11 messages
Drupal\system\Tests\Ajax\AjaxFormCacheTest                    40 passes             1 exceptions  16 messages
- Found database prefix 'test56849268' for test ID 11.
Drupal\system\Tests\Ajax\FrameworkTest                        49 passes                           16 messages

While without concurrency the result is:

Test summary
------------

Drupal\FunctionalJavascriptTests\Ajax\AjaxCallbacksTest        2 passes
Drupal\FunctionalJavascriptTests\Ajax\AjaxFormImageButtonTes   1 passes
Drupal\FunctionalJavascriptTests\Ajax\AjaxFormPageCacheTest    0 passes   1 fails
Drupal\FunctionalJavascriptTests\Ajax\AjaxTest                 2 passes
Drupal\system\Tests\Ajax\AjaxFormCacheTest                    41 passes                           16 messages
Drupal\system\Tests\Ajax\AjaxInGroupTest                      19 passes                            5 messages
Drupal\system\Tests\Ajax\CommandsTest                         92 passes                           50 messages
Drupal\system\Tests\Ajax\DialogTest                           67 passes                           24 messages
Drupal\system\Tests\Ajax\ElementValidationTest                15 passes                            7 messages
Drupal\system\Tests\Ajax\FormValuesTest                       47 passes                           26 messages
Drupal\system\Tests\Ajax\FrameworkTest                        49 passes                           16 messages
Drupal\system\Tests\Ajax\MultiFormTest                        42 passes                           11 messages
Drupal\Tests\Core\Ajax\AjaxCommandsTest                       24 passes
Drupal\Tests\Core\Ajax\AjaxResponseTest                        2 passes
Drupal\Tests\Core\Controller\AjaxRendererTest                  1 passes

I agree with #15, this shouldn't come from the system information - at least not all of it. Most of this should be defined in the theme. So the latest patches aren't very useful I think.

The questions in #17:
1. Using the theme logo as default is probably not going to yield good results for most sites.
2. Uploading one 256x256 file and resizing that sounds like a good solution!

In the attached patch I propose a new service to fill the data, not all of it is implemented yet.

Consider this to be a work in progress and not even close to finished, but just an idea. I would like to get feedback on a couple of things:

1. I first wanted to add a value object for this as well - instead of an array, is that a good idea?.
2. I'm not going to give this any alter hooks or a plugin system to extend, if a site would like to overwrite the default output - they can swap out the service. Or is that making extensibility too hard?
3. Writing the file like I'm doing right now is bad and I know that. Do we want to write this to the root or in the public:// directory?
4. Can we add all of the remaing fields to /admin/appearance/settings

CS fixes + config subscriber to remove the manifest.json when the system settings changes. Also now using the file in the public directory.

I agree with @nod_ on #30. Regarding the start_url , it should be defined site-wide and configurable: to offer an app-like experience, it might point to another page than the homepage or even to a form.

Still to do:
- Set configuration in theme and get information from it.
- add alter hook
- test all the things.

Done in this patch.
- Merge part of the work previous contributors did into this patch.
- Hopefully fix some of the tests
- Automatically generate the file everytime (remove the theme switch)

All information should now come from the correct locations, would love to get feedback before writing tests. I don't think I'll be able to spend a lot of time on this the next couple of days so if someone would like to pick this up - feel free.

We should still fix the icon though.

I also think we should make give this a language-context (if at all possible with the current approach), we should probably also make this vary by active theme.

I think those are good reasons to reconsider @dawehner's point about making this into a route instead of a file.

I don't think we should make the direction configurable, every language object already has that information. So we can reuse it.

1. What is the approach regarding multilingual?
2. Do we generate a manifest.json file per language to facilitate translating the (short) site name and start url?

Reading the spec (https://www.w3.org/TR/appmanifest) it appears one is not limited to one manifest file by design.

1. What is the approach regarding multilingual?
2. Do we generate a manifest.json file per language to facilitate translating the (short) site name and start url?

1. There is no multilingual implementation right now, currently the only thing we do is generate a manifest for the first language that gets accessed. There are 2 options to make this work.
a) Make this into a route instead of generated file, and add cacheability metadata.
b) Generate several files and serve different files based on the cacheability metadata.
2. We should, but don't.

I think the route is a better approach to fix this.

No longer makes the entire core rely on the json encoder service. This is not done yet. The cache metadata isn't bubbled yet.

However, this makes it into a route.

I think the tests shouldn't be as broken anymore.

The tests look to be failing in upgrade path tests, I think that's because the manifest file is added to update.php, we shouldn't do that. No idea how to do that though.

I added an alter hook, as well as a basic test.

Before spending more time on tests, I'd love to get feedback on the implementation as it is now.

Thanks @borisson_!
I'd be happy to give it a try and to share my feedback. I don't have a fresh 8.5 install at hand, so I gave it a try on https://simplytest.me choosing the 8.5x branch with the patch #43, but install fails with this error, which seems related to the patch:

Additional uncaught exception thrown while handling exception.
Original

Symfony\Component\Routing\Exception\RouteNotFoundException: in Drupal\Core\Routing\NullGenerator->getRoute() (line 43 of /home/du09m/www/core/lib/Drupal/Core/Routing/NullGenerator.php).

Drupal\Core\Routing\NullGenerator->getRoute('system.manifest') (Line: 271)
Drupal\Core\Routing\UrlGenerator->generateFromRoute('system.manifest', Array, Array, ) (Line: 753)
Drupal\Core\Url->toString() (Line: 622)
system_page_attachments(Array) (Line: 65)
Drupal\Core\Render\BareHtmlPageRenderer->renderBarePage(Array, 'Choose language', 'install_page', Array) (Line: 76)
Drupal\Core\ProxyClass\Render\BareHtmlPageRenderer->renderBarePage(Array, 'Choose language', 'install_page', Array) (Line: 1004)
install_display_output(Array, Array) (Line: 156)
install_drupal(Object) (Line: 44)

Additional

Symfony\Component\Routing\Exception\RouteNotFoundException: in Drupal\Core\Routing\NullGenerator->getRoute() (line 43 of /home/du09m/www/core/lib/Drupal/Core/Routing/NullGenerator.php).

Drupal\Core\Routing\NullGenerator->getRoute('system.manifest') (Line: 271)
Drupal\Core\Routing\UrlGenerator->generateFromRoute('system.manifest', Array, Array, ) (Line: 753)
Drupal\Core\Url->toString() (Line: 622)
system_page_attachments(Array) (Line: 65)
Drupal\Core\Render\BareHtmlPageRenderer->renderBarePage(Array, 'Error', 'install_page', Array) (Line: 76)
Drupal\Core\ProxyClass\Render\BareHtmlPageRenderer->renderBarePage(Array, 'Error', 'install_page', Array) (Line: 1004)
install_display_output(Array, Array, Array) (Line: 265)
_drupal_log_error(Array, 1) (Line: 566)
_drupal_exception_handler(Object)

Looks like that is because the manifest.json is added on install.php and update.php. We should remove it from those - but like I mentioned in #43 I have no idea how to not do that.

This should fix the problem with update/install.

Test should be all green now.

Thanks for the great progress, really nice :). Like the approach on the route and love to see the tests are in there :).

Some first remarks/questions after some functional testing on simplytest.me.

1. It's not really clear to me once you're configuring your site settings the rest of the settings for the manifest.json are found in the theme settings, maybe we can make that a bit more clear by adding a link to the active theme settings page?
2. As per issue summary: We still need to add a <meta name="theme-color"> tag for branding color to compliment the values set inside manifest.json.
3. I believe we should add some validation to the theme/background color. We can now put anything random in there. This will just render as undefined if it's not a valid color. See below for a test of the manifest.json that came out of my test.

{
  "lang": "en",
  "dir": "ltr",
  "short_name": "Short test name",
  "name": "Manifest test",
  "display": "standalone",
  "icons": [
    {
      "src": "logo.png",
      "sizes": "144x144"
    }
  ],
  "start_url": "\/user\/1",
  "orientation": "landscape",
  "theme_color": "somethingrandom",
  "background_color": "somethingrandom"
}

If there is any help on development I can do let me know! Will leave it on needs review for others to check it out :)

If there is any help on development I can do let me know! Will leave it on needs review for others to check it out :)

The biggest outstanding thing right now is the files, and how we want to generate the different sizes of those, do we want to ship with predefined (locked) image styles or something else?

The other points you brought up are done. The validation is the same as the one color module used, so I moved that to system.

The patch in #51 is wrong, this one is correct.

Should fix the tests again - at least colortest is now green again locally - let's see if the rest is as well.

I successfully installed and tested patch #54 and the UI part seems good to me! A couple remarks:

• There is no description object in the manifest: it seems that adding the site slogan as description if the slogan is set could be a good option, although I'm not sure how it can be translated.
• I changed the default logo.png by an svg or png file with different names using the theme settings in /admin/appearance/settings , and the manifest still links to "src":"logo.png".

@55

I changed the default logo.png by an svg or png file with different names using the theme settings in /admin/appearance/settings , and the manifest still links to "src":"logo.png".

This still needs to be tackled as mentioned in #51, but we need to come up with a solution on how to add the different sizes of the icon. See also the todo in code.

// @todo Get this from the to-be added file field in the theme
+      // configuration.
+      'icons' => [
+        [
+          'src' => 'logo.png',
+          'sizes' => '144x144',
+        ],
+      ],

There is no description object in the manifest: it seems that adding the site slogan as description if the slogan is set could be a good option

I agree we can provide this as an extra, optional (?), object in the manifest. Don't think we should use the site slogan for that. It's more for describing the purpose of the web application rather than showing the slogan of your website.

Self-review, most of these are just docs-updates. I'd be super happy if anyone fixes these.

We can do a description in a later followup, let's try to keep this as small as possible for now. It's already ~30kb and we still need to add better testcoverage + fix the logo. We don't want this to turn into an unreviewable monster later down the line :)

1. +++ b/core/lib/Drupal/Core/Theme/ManifestGenerator.php
   @@ -0,0 +1,107 @@
   +/**
   + * Class ManifestGenerator.
   + */
   +class ManifestGenerator {
   

   This documentation as it right now is useless. It should be expanded.

2. +++ b/core/lib/Drupal/Core/Theme/ManifestGenerator.php
   @@ -0,0 +1,107 @@
   +    $themeSpecificConfig = \Drupal::config($themeName . '.settings');
   ...
   +      $this->themeConfig = \Drupal::config('system.theme.global');
   

   These shouldn't use \Drupal::config but use the configFactory object (that's already injected)

3. +++ b/core/modules/system/src/Controller/ManifestController.php
   @@ -0,0 +1,68 @@
   +/**
   + * Controller for default HTTP 4xx responses.
   + */
   

   Looks like I copied this from another controller, the docs here are wrong.

4. +++ b/core/modules/system/src/Controller/ManifestController.php
   @@ -0,0 +1,68 @@
   +class ManifestController extends ControllerBase {
   

   Are we sure we need to extend controllerBase? If we're not using any of it, we should remove the extend here.

5. +++ b/core/modules/system/src/Controller/ManifestController.php
   @@ -0,0 +1,68 @@
   +   * Return an renderable array of manifest data.
   

   This is wrong, we're not returning a renderable array - we're returning a json response. Let's fix the docs.

6. +++ b/core/modules/system/system.module
   @@ -799,6 +820,20 @@ function system_form_alter(&$form, FormStateInterface $form_state) {
   +function system_valid_hexadecimal_string($color) {
   +  return preg_match('/^#([a-f0-9]{3}){1,2}$/iD', $color);
   +}
   

   Are we sure we want to just move this to a new global function, or should this move to a service somewhere? I guess for now the global function is good enough.

7. +++ b/core/modules/system/tests/src/Kernel/Theme/ManifestGeneratorTest.php
   @@ -0,0 +1,72 @@
   +  public function testGenerateManifest() {
   

   Needs a docblock.

Tried to pitch in a little and fix most of the above.

Still to do:

4.

+++ b/core/modules/system/src/Controller/ManifestController.php
@@ -0,0 +1,68 @@
+class ManifestController extends ControllerBase {

Are we sure we need to extend controllerBase? If we're not using any of it, we should remove the extend here.

6.

+++ b/core/modules/system/system.module
@@ -799,6 +820,20 @@ function system_form_alter(&$form, FormStateInterface $form_state) {
+function system_valid_hexadecimal_string($color) {
+  return preg_match('/^#([a-f0-9]{3}){1,2}$/iD', $color);
+}

Are we sure we want to just move this to a new global function, or should this move to a service somewhere? I guess for now the global function is good enough.

This should fix the broken tests.

1. +++ b/core/lib/Drupal/Core/Theme/ManifestGenerator.php
   @@ -0,0 +1,109 @@
   +    $themeSpecificConfig = $this->configFactory->get($themeName . '.settings');
   +    if (isset($themeSpecificConfig->get()['manifest'])) {
   +      $this->themeConfig = $themeSpecificConfig;
   

   Just a question: Do we really use camelcase variable names? Sorry for my ignorant question.

2. +++ b/core/lib/Drupal/Core/Theme/ManifestGenerator.php
   @@ -0,0 +1,109 @@
   +    return $this->generateData();
   ...
   +  protected function generateData() {
   

   I would have had a method $this->loadThemeConfig and a method doGenerateManifest and then a generateManifest which does both. ```generateData``` isn't really a helpful method name.

3. +++ b/core/lib/Drupal/Core/Theme/ManifestGenerator.php
   @@ -0,0 +1,109 @@
   +    \Drupal::moduleHandler()->alter('manifest', $data);
   

   Is there a reason this is not injected?

4. +++ b/core/modules/color/color.module
   @@ -346,9 +346,12 @@ function color_palette_color_value($element, $input, FormStateInterface $form_st
   + * @deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.x. Use
   ...
    function color_valid_hexadecimal_string($color) {
   -  return preg_match('/^#([a-f0-9]{3}){1,2}$/iD', $color);
   +  return system_valid_hexadecimal_string($color);
    }
   

   Let's ensure to add a @trigger_error as I guess we treat this, just like everything else, as some sort of "API"?

5. +++ b/core/modules/system/src/Controller/ManifestController.php
   @@ -0,0 +1,72 @@
   +    $manifestData = $this->manifestGenerator->generateManifest($themeName);
   ...
   +    $response->addCacheableDependency(CacheableMetadata::createFromRenderArray($manifestData));
   +    unset($manifestData['#cache']);
   

   This render array bit feels so weird. I would rather introduce a new custom value object which implements \Drupal\Core\Cache\RefinableCacheableDependencyInterface and stores the needed data.

6. +++ b/core/modules/system/system.module
   @@ -675,6 +675,27 @@ function system_page_attachments(array &$page) {
   +  if (!\Drupal::service('router.admin_context')->isAdminRoute() && ($routeName !== 'system.db_update' && $routeName !== '<none>')) {
   

   Can't you use strpos($request->getScriptName(), 'update.php') === FALSE or so?

7. +++ b/core/modules/system/system.module
   @@ -799,6 +820,20 @@ function system_form_alter(&$form, FormStateInterface $form_state) {
   +function system_valid_hexadecimal_string($color) {
   +  return preg_match('/^#([a-f0-9]{3}){1,2}$/iD', $color);
   +}
   

   A random suggeston: Could we add a Drupal\Component\Utility helper class instead? This sounds for me like something which really doesn't have to exist in system module.

Thanks so much for the review daniel, that really helps!

1. We don't, fixed.
2. I agree the methodname was bad, I think I understood everything correctly.
3. Fixed
4. Added the trigger_error
5. I agree, I already wondered about that in #25. Fixed.
6. Done.
7. Sure, however I'm not sure if we should add a new class just for this. Not yet done.

Test should be green again now. I decided to remove the part about the file icon, we can do that as a followup later.

Still todo:

Move the system_valid_hexadecimal_string to a Drupal\Component\Utility helper

Moved the system_valid_hexadecimal_string to a Drupal\Component\Utility helper.

Used Drupal\Component\Color for that, there was already a validateHex function in there, but that accepted strings like "cba" unfortunately that is not strict enough for the manifest.json. Also made sure the tests for both functions reflects the above use case.

1. +++ b/core/lib/Drupal/Component/Utility/Color.php
   @@ -30,6 +30,21 @@ public static function validateHex($hex) {
   +   * Determines if a hexadecimal CSS color string is valid in a strict way,
   +   * so it should start of with a # followed by at least 3 characters.
   

   The first line of documentation should be only 80 characters long and the rest should be in a new paragraph. Can we rewrite this?

   Determines if a hexadecimal CSS color is valid in a strict way.
   
   It should start with a # and be followed by either 3 or 6 hexacecimal characters.
   

2. +++ b/core/lib/Drupal/Component/Utility/Color.php
   @@ -30,6 +30,21 @@ public static function validateHex($hex) {
   +   *   TRUE if the string is a valid hexadecimal CSS color string, or FALSE if it
   +   *   isn't.
   

   This goes over the 80 cols line.

I like the new tests, great job on those!

Thanks a lot for the review!

Updated the documentation as per your comments.

Created the patch against 8.6.x

Notable changes (also see interdiff):

-      $config->set('manifest.' . Unicode::substr($key, 9), $value);
+      $config->set('manifest.' . mb_substr($key, 9), $value);

Necessary because of: https://www.drupal.org/project/drupal/issues/2849669

Still todo

According to the Lighthouse validation there should be at least two icon sizes rendered:

+    'icons' => [
+       [
+        'src' => url(to/image/-512.png),
+        'sizes' => '512x512',
+        'type' => 'image/png',
+      ],
+      [
+        'src' => url(to/image/-192.png'),
+        'sizes' => '192x192',
+        'type' => 'image/png',
+      ],

Can somebody maybe elaborate on how to tackle this best?
Imagestyle? Two separate upload fields with size restrictions?

I think an image style is the best idea, 2 file fields sounds like making it a lot harder for people when it's not really needed.

Also removed the needs tests tag, I think this has sufficient coverage as-is.

Unit tests did run and passed. Functionality test setting the parameters from the manifest.json file works flawlessly.

A remark about the position of the site manifest parameters fieldset within the basic settings page though. Maybe it would be a good idea to not to nest within the site details fieldset?

This needs API documentation for hook_manifest_alter().

The manifest file is being served with a Content-type: application/json header, but it's supposed to be application/manifest+json if we're following the W3C Web App manifest working draft.

That draft also describes the file extension as .webmanifest but we currently jdon't have a file extension.

We can change the header really easily, however creating a file is not possible, we tried that earlier and that was harder to fix for multilingual.

Using a JSON is an acceptable file extension for Web Manifest. The file's official structure is JSON so sites like MDN docs often suggest that JSON file extension is a safe convention.

I cleanly applied this to latest 8.7.x and it's still working. I found one issue while clicking through and trying out the admin UI:

+++ b/core/modules/system/config/install/system.site.yml
@@ -10,3 +10,8 @@ admin_compact_mode: false
+  display: 'browser'

I see that browser is the default Display setting in the YML but when I test this patch on a fresh installation I get "Fullscreen" as my default value when I load the form and save.

So in terms of Lighthouse audit checkpoints, the two images sized 192/512 are necessary to make the site installable by default. In the contrib module we just use the Druplicon and I figure that's ok for core to do as well. The PNG inside core/misc is 88x110 but checking in two additional copies at the correct size seems like an easy path to "out of the box" functionality.

Someone mentioned writing docs for the manifest alter hook. That's something I'm happy to do. Does it need to be in this patch or on a d.o page somewhere?

core.api.php could be a good start perhaps? not sure.

+++ b/core/lib/Drupal/Core/Theme/ManifestGenerator.php
@@ -0,0 +1,121 @@
+  /**
+   * Load the theme configuration.
+   *
+   * @param string $themeName
+   *   The theme name to generate data for.
+   *
+   * @return \Drupal\Core\Config\ImmutableConfig
+   *   The configuration of the theme.
+   */
+  protected function loadThemeConfig($themeName) {
+    $theme_specific_config = $this->configFactory->get($themeName . '.settings');
+    if (isset($theme_specific_config->get()['manifest'])) {
+      return $theme_specific_config;
+    }
+    return $this->configFactory->get('system.theme.global');
+  }

This looks generic enough, and useful not only for manifests, can we refactor/add it somewhere more accessible?

@Manuel Garcia: I don't think we should do that, at least not yet. Opening this up to a general API might be useful, but this is a DX improvement we can always add somewhere in the future. For now, I'd like to keep the API surfuce of this patch as small as possible.

Is there any reason why the ManifestGenerator can't just have a Drupal\Core\Theme\ThemeSettings member?

I don't really understand #82.

1. +++ b/core/lib/Drupal/Core/Theme/Manifest.php
   @@ -0,0 +1,54 @@
   +    $this->data = $data;
   ...
   +    return $this->data;
   

   If data is just a keyed array, we're not really making the most of php5.4 memory optimisations.

   Are there known keys here we should be making properties on the object?

   If not - what is the value object adding that an array doesn't provide? Is it just to provide the cacheability methods?

2. +++ b/core/lib/Drupal/Core/Theme/ManifestGenerator.php
   @@ -0,0 +1,121 @@
   +class ManifestGenerator {
   

   Should we have a interface for this to allow alternative implementations?

3. +++ b/core/lib/Drupal/Core/Theme/ManifestGenerator.php
   @@ -0,0 +1,121 @@
   +    if (isset($theme_specific_config->get()['manifest'])) {
   

   would
   $theme_specific_config->get('manifest') work?

4. +++ b/core/modules/color/color.module
   @@ -346,9 +347,13 @@ function color_palette_color_value($element, $input, FormStateInterface $form_st
   + * @deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.x. Use
   + * system_valid_hexadecimal_string instead.
   ...
   +  @trigger_error('color_valid_hexadecimal_string is deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.x. Use Drupal\Component\Utility\Color::validateStrictHex instead.', E_USER_DEPRECATED);
   

   these need a link to a change record

5. +++ b/core/modules/system/src/Form/SiteInformationForm.php
   @@ -118,12 +120,51 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +      '#description' => $this->t("The site name."),
   ...
   +      '#description' => $this->t("The short name of the site."),
   

   no need for " here, should be ' for consistency?

6. +++ b/core/modules/system/src/Form/SiteInformationForm.php
   @@ -118,12 +120,51 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +        'fullscreen' => 'Fullscreen',
   +        'standalone' => 'Standalone',
   +        'minimal-ui' => 'Minimal UI',
   +        'browser' => 'Browser',
   

   shouldn't these be passed through $this->t() ?

7. +++ b/core/modules/system/src/Form/ThemeSettingsForm.php
   @@ -367,6 +369,37 @@ public function buildForm(array $form, FormStateInterface $form_state, $theme =
   +      '#title' => t('Orientation'),
   ...
   +        'portrait' => t('Portrait'),
   +        'landscape' => t('Landscape'),
   

   Should use $this->t()?

8. +++ b/core/modules/system/src/Form/ThemeSettingsForm.php
   @@ -420,6 +453,14 @@ public function validateForm(array &$form, FormStateInterface $form_state) {
   +      if (in_array($key, ['manifest_background_color', 'manifest_theme_color']) && !empty($color) && !Color::validateStrictHex($color)) {
   

   Can we use the third argument to in_array here. We're comparing strings, so we should be strict.

   https://3v4l.org/949aT

9. +++ b/core/modules/system/system.module
   @@ -675,6 +675,33 @@ function system_page_attachments(array &$page) {
   +      ->toArray();
   

   e.g. this would be an ideal candidate for a ->getThemeColor method

10. +++ b/core/tests/Drupal/Tests/Component/Utility/ColorTest.php
    @@ -12,6 +12,97 @@
    +    // Any invalid arguments should throw an exception.
    ...
    +    // Any invalid arguments should throw an exception.
    

    Do they actually throw an exception?

We also need docs for the new hook, and the update path for the new config (with a test)

Looks great though, really valuable feature

Thanks @larowlan for the review,

#84.1 - Probably, let's identify these.
#84.2 - Good idea, doing it here please have a look.
#84.3 - I think it should, doing so in this patch.
#84.4 - Yes, we need to write it first, adding the tag.
#84.5 - Good catch, done.
#84.6 - Yes they should, done.
#84.7 - Good catch, done.
#84.8 - Yup, done.
#84.9 - Possibly, lets first sort out #84.1
#84.10 - I don think so, this looks to test just Color::validateStrictHex() which returns the value from the preg_match(bool). Changed the comment to something more generic.

OK that interdiff came out very wrong, here is the good one.

Sigh back to the old way of generating patches then. Interdiff on #86 is good.

Still needs work for these as far as I can tell:

• #72
• #84.1/#84.9
• Write change record and link to it, see #84.4
• Write upgrade path & tests

We need to address WCAG SC 1.3.4 Orientation. Currently this patch prevent site-builders from producing a website that conforms to WCAG level AA.

I propose (maybe insist) we remove the orientation key entirely. Locking the orientation is justified only in rare cases, and including this setting seems irresponsible IMO. The alter hook provides a way to lock the orientation key for sites which can justify it.

I'll expand on this comment later. I'm on a rural bus journey right now.

@borisson_ Sorry I was too brief in #82.

It was about the loadThemeConfig() method in #80. Is this new method really necessary? There's already a ThemeSettings class which sorts out the global/theme-specific settings. Can manifestgenerator use this existing feature instead?

The "display" property also has some accessibility concerns.

When a user views a website in a browser, they have lots of useful tools built in to the browser: bookmarking a URL, find in page, share a URL to another app, Firefox add-ons for dictionary look-ups, reader mode, etc..

But when they save a "page" to the homescreen, the PWA manifest "display" property kicks in, and the user may loose access to some browser tools which they depend on - e.g. with "minimal-ui", PWAs on Android/Chrome do not offer bookmarking URLs. I'm highly skeptical of why many web sites would need to do this at all.

I'm not sure what happens if users add a PWA to their homescreen from Android/Firefox, but I'm concerned users will lose the ability to use Firefox addons which they rely on (like dictionary look-ups).

I think use of the "display" property could present problems for users with a large range of cognitive/learning impairments. I'll keep looking for articles which can provide more info on this. I don't think we'll find an outright WCAG failure here, but it's something we can provide guidance about for site builders, in the manifest settings form and help topics.

To address #91 concerns: Orientation is just specifying the default orientation. It's not a locking mechanism, rather a signal for what a given site considers its default or most useful orientation. There are more than just two options. Would setting any as a default alleviate the concerns you have?

Other options listed at https://developer.mozilla.org/en-US/docs/Web/Manifest#orientation

Per #93 The display property is similar in that it's only as restrictive to accessibility as the individual settings allow. We can choose the most conservative default of browser and it won't be any different than a normal web page.

If there's really a measurable problem with including these two properties by default, we could add a None option which still makes them easily adjustable without forcing one particular implementation on folks. See an example manifest with just the bare minimum of meta-data (Example1): https://www.w3.org/TR/appmanifest/#example-manifests

@andrewmacpherson do you have some links we could read explaining why these properties have negative effects on a11y? This is the first I've heard of it and I'm keen to learn more in case I need to adjust something in the contrib module.

This is just a minor observation, found while looking over the patch

t() is a antipattern, I have converted a couple of instances into $this->t().. only where we were adding new code.

Just wanting to test #95

High-level review.

1. +++ b/core/lib/Drupal/Core/Theme/Manifest.php
   @@ -0,0 +1,54 @@
   + * A value object containing all data for the manifest.
   + */
   +class Manifest implements RefinableCacheableDependencyInterface {
   ...
   +  public function overwriteWithNewData($data) {
   +    return new static($data);
   +  }
   

   Implementing RefinableCDI makes this a mutable value object. In every other way this is immutable.

   Let's keep it immutable.

2. +++ b/core/modules/system/config/schema/system.schema.yml
   @@ -44,6 +44,22 @@ system.site:
   +        start_url:
   ...
   +          label: 'Default start page'
   

   Inconsistent: URL vs page.

3. +++ b/core/modules/system/config/schema/system.schema.yml
   @@ -44,6 +44,22 @@ system.site:
   +        display:
   +          type: string
   +          label: 'Display'
   

   Display what? Based on the name, I though this was going to be a boolean.

4. +++ b/core/modules/system/config/schema/system.schema.yml
   @@ -44,6 +44,22 @@ system.site:
   +        short_name:
   +          type: label
   +          label: 'Short name'
   +        name:
   +          type: label
   +          label: 'Short name'
   

   How can these have the same label?

5. +++ b/core/modules/system/src/Controller/ManifestController.php
   @@ -0,0 +1,71 @@
   +    $response = new CacheableJsonResponse();
   +    $response->addCacheableDependency($manifestData);
   +    $response->setData($manifestData->toArray());
   +    return $response;
   +  }
   

   🎉

6. +++ b/core/modules/system/tests/modules/manifest_test/manifest_test.module
   @@ -0,0 +1,17 @@
   +/**
   + * Implements hook_manifest_alter().
   + */
   +function manifest_test_manifest_alter(&$variables) {
   ...
   +    $variables = $variables->overwriteWithNewData($new_data);
   

   This is lacking the ability to influence cacheability.

The IS mentions icons, but there's no icons in the latest patch.

No activity for a long time, but brought to my attention by @borisson_, so thanks for the opportunity!

Attached is a new patch with the feedback by Wim + some additional improvements. I made the naming of the start_url more consistent in the SiteInformationForm.

I was also wondering if the manifest config this patch adds in system.site.yml must also be added to the Standard install profile system.site.yml config file? (and maybe the other install profiles as well?)

Re-rolled the #100 patch against 8.8.x branch.

Thanks for the re-role @yogeshmpawar!
Hope we can move this forward now.

A) Just pull out the "additional coding standard errors" out from the test artefacts

/var/lib/drupalci/workspace/jenkins-drupal_patches-102226/source/core/lib/Drupal/Component/Utility/Color.php ✗ 1 more
line 38	Doc comment for parameter $color does not match actual variable name $hex
/var/lib/drupalci/workspace/jenkins-drupal_patches-102226/source/core/tests/Drupal/Tests/Component/Utility/ColorTest.php ✗ 2 more
20	Expected "bool" but found "boolean" for parameter type
65	Expected "bool" but found "boolean" for parameter type 

B) Also there are out of date deprecation notices being added.

+ * @deprecated in Drupal 8.5.x and will be removed before Drupal 9.0.x. Use
+ * system_valid_hexadecimal_string instead.

The is movement to standardise this text. Here is a example from
https://www.drupal.org/core/deprecation

@trigger_error('baz() is deprecated in drupal:8.3.0 and is removed from drupal:9.0.0. Use \Drupal\Foo\Bar::baz() instead. See http://drupal.org/node/the-change-notice-nid', E_USER_DEPRECATED);

(b.2) drupal:8.8.0 is now the preferred label.

(b.3) "is removed from" is intentional so that code can linger in drupal:9.0.0 and be deleted in later minor releases. without warning.

I hope this helps.

Thanks @martin107 for the review, I will take a look

New patch with the feedback from @martin107.

I changed all the 'Boolean' occurrences to 'bool'. So it is consistent with the rest of Core.

My time is limited at the moment , but digging in the build artifacts

Error: Call to undefined method Drupal\Core\Theme\Manifest::addCacheContexts()

so that looks like a missing use statement, as addCacheContexts() is not under the Drupal\Core\Theme namespace

I hope this helps

Error: Call to undefined method Drupal\Core\Theme\Manifest::addCacheContexts() in Drupal\Core\Theme\ManifestGenerator->doGenerateManifest() (line 95 of core/lib/Drupal/Core/Theme/ManifestGenerator.php).
Drupal\Core\Theme\ManifestGenerator->doGenerateManifest() (Line: 71)
Drupal\Core\Theme\ManifestGenerator->generateManifest('classy') (Line: 693)
system_page_attachments(Array) (Line: 297)
Drupal\Core\Render\MainContent\HtmlRenderer->invokePageAttachmentHooks(Array) (Line: 273)
Drupal\Core\Render\MainContent\HtmlRenderer->prepare(Array, Object, Object) (Line: 117)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object, Object) (Line: 90)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object, 'kernel.view', Object)
call_user_func(Array, Object, 'kernel.view', Object) (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.view', Object) (Line: 156)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 191)
Drupal\page_cache\StackMiddleware\PageCache->fetch(Object, 1, 1) (Line: 128)
Drupal\page_cache\StackMiddleware\PageCache->lookup(Object, 1, 1) (Line: 82)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 693)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)"
    at Object.Test page (/var/www/html/core/tests/Drupal/Nightwatch/Tests/exampleTest.js:15:15)
    at _combinedTickCallback (internal/process/next_tick.js:132:7)

This resolves what I described above

CacheableDependencyInterface becomes RefinableCacheableDependencyInterface

when to use a RefinableCacheableDependencyInterface?

The following interface should be implemented by any class whose business logic implies dealing at runtime with variations of a canonical object:

https://www.drupal.org/node/2525764

Anyway locally ManifestGeneratorTest now passes.

Wim's remark in #97 was to remove RefinableCacheableDependencyInterface. Because that makes this VO mutable and it was immutable before. I don't see how we can keep this immutable without creating a new way to pass in the cacheability metadata.

So I think that means #108 is a good start. I think the next thing we should change is to allow for the cache data to also be passed to the new value object as well.

+++ b/core/lib/Drupal/Core/Theme/Manifest.php
@@ -0,0 +1,54 @@
+  public function overwriteWithNewData($data) {
+    return new static($data);
+  }

@borisson_ Oh ground swallow my up ... thanks for pointing out my mistake.

I should explain a few judgement calls I have made.

A)

Regarding the manifest.json file, I want to talk about mitigating the differences between what the spec says and what the disparate implementations do.

1) Name and manifest_version are the only required manifest key value pairs. So I have added "manifest_version" as a new form entry and made the 'name' field required.

Browser are pragmatic - If manifest_version is missing form the file then a default is assumed - but I want to follow the spec, while recognising it is not actually needed.

2) I have reworked the manifest_generator so that if, for example, theme_color is missing then instead of the key value pair being
'theme_color' => ''

we now omit the whole thing.

3) Given 2 - theme_color if it is blank then $page['#attached']['html_head'][] is no longer modified.

B) I went looking at the manifest.json generated by a 'standard' and 'unami' install. lots of value were null. Now placeholder values appear.

C) From now on php7.0.3 is required. For new code it is time to specify a return type

Here is a example of the change I have made to ManifestController. We can now mandate a CacheableJsonResponse. yay for 2019

  /**
   * Returns JSON data for the manifest.json file.
   *
   * The actual data is generated by \Drupal\Core\Theme\ManifestGenerator.
   *
   * @return \Drupal\Core\Cache\CacheableJsonResponse
   *   The JSON response.
   */
  public function manifest() : CacheableJsonResponse {}

So on my todo list

Returning to the difference between spec and implementation.

D) Chrome generates lots of warning if it sees missing (optional) png definitions for critical icon sizes.

E) I have made the manifest permanently cached. I want to option to set it to say a week?

I have not work on my todo list from above, I got sidelined with other issues that arose.
I still plan to work on the todo list.

A) I have converted a kernel test to a unit test.. something I had better justify.

1) Debugging slow tests is painful. Tests that run in milliseconds rather than 5 mins make me happy.
2) The additional assurance the kernel test gave was proving the wiring of the service into core etc
are all duplicated. We have extensive functional testing for that.

B) ThemeSettingsForm had a bug in it. Color was not found.

+use Drupal\Component\Utility\Color;

C) Some optional field are pulled down from a list .. I looked at the spec and added missing values.

D) I add a "--Leave Empty --" pulled down optional so that a user can omit the field.

Coding Standard fix up.

Hi, will this feature allow PUSH notification and offline content ? Thank you

All the test fails seem to have common error. so I fixed up Drupal\Tests\file\FunctionalJavascript\FileFieldWidgetTest
and testbot will tell me what is left.

@zenimagine

Hi, will this feature allow PUSH notification and offline content ?

No, those are both big topics .. which we as community need to keep upto date with - just not here.

storing content offline is a massive topic..
In general all solutions of that type need to register a service worker with the browser .. JS code in the browser to act a network proxy .. code intercepting all network requests and providing faked network data from a local store when offline. All core can do there is provide hooks for site specific/ theme specific/ contrib module specific logic to make decisions about what to cache etc. ( by hooks I mean that very loosely in a class driven sense. )

I hope that makes sense.

When I look for further work after this issue. Adding 'content_scripts' to the manifest is a different large topic.

Updated patch with an interdiff.

@yogeshmpawar
Thanks for fixing my errors. - that was helpful

I think I might re-work your test changes a little. It is better to expected the test to return something unexpected something that one can't have come from a default hidden in the system.

In short, I want to set the expected return value to be 'llama'

I grabbing this back to signal that I am actively working on this.

I have a support for icons partially working .. nothing ready for show and tell yet.

I added the first cut versions of icons support -- in the manifest file.

A) Added a table to ThemeSettingsForm -- each table row contains a fieldset holding a icon datablock.

B) The schema has been updated to hold a unlimited list of icon data

C) The icon section is now output in the manifest.json file.

I have work to do

D) The form needs 'delete' and 'add new icon' actions associated with the icon block.

E) The icons are cascaded so the table need to be draggable.

I have split off the umami design decisions related to the mobile and desktop icons into a separate issue

So that the umami contributors can have their say.

From there perspective - we provide the mechanism, they the icons.

While I think about it, whatever I set for the default bartik/core theme has branding implications -- which is a whole discussion to itself.

D) The form needs 'delete' and 'add new icon' actions associated with the icon block.

E) The icons are cascaded so the table need to be draggable.

I agree, but this will make this patch need JS-based testing as well. I'm not sure if we need to do that already in this issue (that has been dragging for a while), or if we should do that in a followup.