Fix description in index.html / manifest.json getting double quoted #131842

navaronbracke · 2023-08-03T11:35:11Z

This PR adjusts the quoting of the project description not not apply twice in the index.html / manifest.json of web builds.

List which issues are fixed by this PR. You must list at least one issue.
Fixes #131834

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I signed the CLA.
I listed at least one issue that this PR fixes in the description above.
I updated/added relevant documentation (doc comments with ///).
I added new tests to check the change I am making, or this PR is test-exempt.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2023-08-03T13:00:03Z

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

packages/flutter_tools/test/commands.shard/permeable/create_test.dart

andrewkolos

I wonder if there's a better way to approach the problem.

Instead of undoing the YAML-string-escaping when writing to a non-YAML file, should we just delay YAML-escaping until we are writing to YAML files?

It seems odd to YAML-escape string items when creating context, when these items might not be used for only YAML files.

I will bring this up during weekly tool PR review (Aug 10).

andrewkolos · 2023-08-07T21:23:09Z

packages/flutter_tools/lib/src/template.dart

Should we have this comment explain where the extra quotes came from?

Makes sense to do that, then a future reader will also see why we had this bug in the first place :)

Since the 'extra' quotes have been eliminated from createTemplateContext(), I've adjusted the comment to mention why we add quotes instead.

packages/flutter_tools/test/commands.shard/permeable/create_test.dart

andrewkolos · 2023-08-07T21:42:59Z

packages/flutter_tools/lib/src/template.dart

Note for myself when doing my next review pass:

Is it safe to modify context here? Will this result in bugs if we change the rendering order of the template files? Instead of writing to context['description'], maybe we should just make a local copy of it.

Then we'd have to make sure that the template renderer gets the local when calling renderString(), which might not be as flexible if other things need to get passed down in the future.

I think your idea of delaying the YAML escaping, until we have a YAML file to create makes more sense here.
Then it is up to the template renderer to decide when to escape what.

Made a local copy of the context when writing to the pubspec.

navaronbracke · 2023-08-10T17:55:48Z

@andrewkolos I had a second look at how the template is rendered.

So we pass things to mustache, that takes care of rendering to the template.

Apparently it has a htmlEscapes property for the template renderer.

Wouldn't it be better if it did the same for yaml?
Then we can also eliminate the manual escapeYamlString for the .metadata file (for the channel & revision variable)

Ref: https://github.com/jonahwilliams/mustache/blob/c4344e0dd45f6605758eb11aa4837859e2c055f0/README.md?plain=1#L39

Edit: Since that is a third-party dependency, I decided to go ahead and adhere to your original feedback. We can always revisit if needed.

andrewkolos

Hello, sorry for being absent. I was out of action towards the end of last week. I requested one change.

andrewkolos · 2023-08-14T20:02:39Z

packages/flutter_tools/lib/src/template.dart

Is the second condition redundant with the first? Also, consider making the leading comment more concise.

Suggested change

// YAML files require that their input is properly escaped with quotes.

// Adjust the current context to escape the needed inputs, before writing to the template.

if (finalDestinationFile.path.endsWith('.yaml')) {

if (finalDestinationFile.path.endsWith('pubspec.yaml')) {

// Grab a copy of the context, since it is used to render other templates as well.

localContext = Map<String, Object?>.of(localContext);

final String? description = localContext['description'] as String?;

if (description != null && description.isNotEmpty) {

localContext['description'] = escapeYamlString(description);

}

}

}

// Ensure inputs are escaped when necessary.

if (finalDestinationFile.path.endsWith('.yaml')) {

// Use copy of the context, since the original is used in rendering other templates.

localContext = Map<String, Object?>.of(localContext);

final String? description = localContext['description'] as String?;

if (description != null && description.isNotEmpty) {

localContext['description'] = escapeYamlString(description);

}

}

No worries :)

Yeah, I was a bit conservative about "what if we need to handle other YAML files in the future", but that doesn't happen often.

Adjusted the condition and updated the comment.

andrewkolos

LGTM

navaronbracke · 2023-08-15T07:50:04Z

Rebased on ToT to kick off the Google Testing check again

andrewkolos · 2023-08-17T04:44:41Z

@christopherfujino Are you able to take a look at this?

christopherfujino · 2023-08-17T16:08:50Z

packages/flutter_tools/lib/src/template.dart

+        Map<String, Object?> localContext = context;
+
+        // Ensure inputs are escaped when necessary.
+        if (finalDestinationFile.path.endsWith('.yaml')) {
+          // Use a copy of the context,
+          // since the original is used in rendering other templates.
+          localContext = Map<String, Object?>.of(localContext);
+
+          final String? description = localContext['description'] as String?;
+
+          if (description != null && description.isNotEmpty) {
+            localContext['description'] = escapeYamlString(description);
+          }
+        }


I'm a little worried that we're conditionally mutating the localContext variable, and this behavior may be obscured and forgotten in the future. Instead, can you pull the escaping logic into a helper function with a dartdoc, so it's more clear when and why we would be escaping, and with which copy?

Suggested change

Map<String, Object?> localContext = context;

// Ensure inputs are escaped when necessary.

if (finalDestinationFile.path.endsWith('.yaml')) {

// Use a copy of the context,

// since the original is used in rendering other templates.

localContext = Map<String, Object?>.of(localContext);

final String? description = localContext['description'] as String?;

if (description != null && description.isNotEmpty) {

localContext['description'] = escapeYamlString(description);

}

}

final Map<String, Object?> localContext = finalDestinationFile.path.endsWith('.yaml')

? _createEscapedContextCopy(context)

: context;

And then the inner block moved into Map<String, Object?> _createEscapedContextCopy(Map<String, Object?> oldContext)

christopherfujino

LGTM thanks for this fix!

TahaTesser · 2023-08-18T07:35:27Z

Thanks for the fix @navaronbracke !

…quoted (flutter/flutter#131842)

github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Aug 3, 2023

navaronbracke marked this pull request as draft August 3, 2023 12:59

navaronbracke marked this pull request as ready for review August 3, 2023 13:19

navaronbracke changed the title ~~Fix description in index.html getting double quoted~~ Fix description in index.html / manifest.json getting double quoted Aug 3, 2023

andrewkolos self-requested a review August 3, 2023 20:30

navaronbracke commented Aug 4, 2023

View reviewed changes

packages/flutter_tools/test/commands.shard/permeable/create_test.dart Outdated Show resolved Hide resolved

andrewkolos requested changes Aug 7, 2023

View reviewed changes

navaronbracke requested a review from andrewkolos August 11, 2023 09:51

andrewkolos requested changes Aug 14, 2023

View reviewed changes

navaronbracke requested a review from andrewkolos August 14, 2023 20:35

andrewkolos approved these changes Aug 14, 2023

View reviewed changes

andrewkolos requested a review from christopherfujino August 14, 2023 20:45

navaronbracke added 4 commits August 15, 2023 09:48

fix description in index.html getting double quoted

c514bd8

fix description in manifest.json getting double quoted

e1f5f41

delay escaping the project description until writing to pubspec template

804929f

address feedback

776a44e

navaronbracke force-pushed the fix_index_content_quotes branch from 3438c6b to 776a44e Compare August 15, 2023 07:48

christopherfujino reviewed Aug 17, 2023

View reviewed changes

use utility function

ca68388

navaronbracke requested a review from christopherfujino August 17, 2023 17:03

christopherfujino approved these changes Aug 17, 2023

View reviewed changes

christopherfujino and others added 2 commits August 17, 2023 10:13

Merge branch 'master' into fix_index_content_quotes

2ccf6d9

Merge branch 'master' into fix_index_content_quotes

b1e7eda

andrewkolos added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 18, 2023

auto-submit bot merged commit 03664d0 into flutter:master Aug 18, 2023

navaronbracke deleted the fix_index_content_quotes branch August 18, 2023 06:28

engine-flutter-autoroll mentioned this pull request Aug 18, 2023

Roll Flutter from 2502b51f86c1 to 633b1ce402c7 (39 revisions) flutter/packages#4740

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 18, 2023

03664d0 Fix description in index.html / manifest.json getting double …

ec92dc6

…quoted (flutter/flutter#131842)

engine-flutter-autoroll mentioned this pull request Aug 18, 2023

Roll Flutter from 2502b51f86c1 to 312ef541156c (40 revisions) flutter/packages#4741

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 18, 2023

03664d0 Fix description in index.html / manifest.json getting double …

a79745f

…quoted (flutter/flutter#131842)

engine-flutter-autoroll mentioned this pull request Aug 18, 2023

Roll Flutter from 2502b51f86c1 to b52297da0620 (41 revisions) flutter/packages#4742

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 18, 2023

03664d0 Fix description in index.html / manifest.json getting double …

9618c61

…quoted (flutter/flutter#131842)

engine-flutter-autoroll mentioned this pull request Aug 18, 2023

Roll Flutter from 2502b51f86c1 to 5a556f8ecde6 (42 revisions) flutter/packages#4743

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 18, 2023

03664d0 Fix description in index.html / manifest.json getting double …

f24cdb7

…quoted (flutter/flutter#131842)

TahaTesser mentioned this pull request Aug 23, 2023

flutter create on Flutter 3.14.0-0.1.pre generates invalid index.html and manifest.json #133110

Closed

2 tasks

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 15, 2023

03664d0 Fix description in index.html / manifest.json getting double …

bc2733e

…quoted (flutter/flutter#131842)

Fix description in index.html / manifest.json getting double quoted #131842

Fix description in index.html / manifest.json getting double quoted #131842

Uh oh!

Conversation

navaronbracke commented Aug 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented Aug 3, 2023

Uh oh!

Uh oh!

andrewkolos left a comment

Choose a reason for hiding this comment

Uh oh!

andrewkolos Aug 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navaronbracke Aug 8, 2023

Choose a reason for hiding this comment

Uh oh!

navaronbracke Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andrewkolos Aug 7, 2023

Choose a reason for hiding this comment

Uh oh!

navaronbracke Aug 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

navaronbracke Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

navaronbracke commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewkolos left a comment

Choose a reason for hiding this comment

Uh oh!

andrewkolos Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

navaronbracke Aug 14, 2023

Choose a reason for hiding this comment

Uh oh!

andrewkolos left a comment

Choose a reason for hiding this comment

Uh oh!

navaronbracke commented Aug 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewkolos commented Aug 17, 2023

Uh oh!

christopherfujino Aug 17, 2023

Choose a reason for hiding this comment

Uh oh!

christopherfujino left a comment

Choose a reason for hiding this comment

Uh oh!

TahaTesser commented Aug 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

navaronbracke commented Aug 3, 2023 •

edited

Loading

andrewkolos Aug 7, 2023 •

edited

Loading

navaronbracke Aug 8, 2023 •

edited

Loading

navaronbracke commented Aug 10, 2023 •

edited

Loading

navaronbracke commented Aug 15, 2023 •

edited

Loading