🇺🇸United States @benjifisher

benjifisher → created an issue.

tekNorah → credited benjifisher → .

For the record, the attendees at the usability meeting (Comment #64) were @AaronMcHale, @benjifisher, @rkoller, @shaal, @simohell, @skaught, and @worldlinemine.

We all agreed that the two submit buttons ("Save translations" and "Delete translations") were confusing. Given that, I do not think we should implement a confirmation form.

Deleting a translatable string is temporary. As soon as someone visits a page where the string is used, the string is re-added to the list of strings available for translation. This whole approach seems confusing from a usability perspective.

Opinions at the usability meeting were divided. Some of us were in favor of each of these proposals:

1. Close this issue as "won't fix".
2. Add a checkbox as in the current MR, but not a second submit button. When the form is submitted, the checkbox has the same effect as deleting the text in the "Translation for ..." column.
3. Add help text to clarify that deleting the translation means the source string will be used.

benjifisher → created an issue.

Now that this issue is Fixed, I will tag an RC release.

I merged 🐛 Typogrify does not validate its settings Needs review , so this issue is now un-postponed.

I still need to fix the deprecations that I mentioned in #6. After some discussion on Slack (thanks to @mglaman, @bbrala, and @Gábor Hojtsy) I opened https://github.com/phpstan/phpstan/issues/10977, where @ondrejmirtes explained that I would need to run PHPStan at level 7 or higher to detect those deprecations.

Since this issue is blocking 📌 Automated Drupal 11 compatibility fixes for typogrify Postponed , I am merging the changes now. I will still be glad to get reviews of the MR.

I added some comments to MR 7882 (targeting 10.4.x). Some or all may also apply to MR 7261.

@quietone, can you explain in the issue summary what should be done with the two PRs? In particular, how do we end up with the deprecation notice in the correct branch or branches?

Please also check the CR and the deprecation notice to make sure that they reference the correct branches.

Is it too late to deprecate this method and remove it in the 11.x branch? Is it even in scope for this issue? If so, then please explain in the issue description. Or maybe it is in scope, but too late to remove it in 11.x, in which case it might be simplest to defer it to a follow-up issue.

The issue summary says something about the changes to the PHPStan baseline. I think that needs to be updated, at least for the 10.4.x branch.

The screenshot in the issue summary needs an update. The page title now includes the field type. For example, when adding a boolean field, the page title is "Boolean settings for Basic page Add to Default shortcuts", not "Field settings for Basic page Add to Default shortcuts".

After I save the new field, I can still edit it. When I do that, the page title still includes the field type (like "Boolean") instead of the field name (like "Test boolean field"). The same thing happens with existing fields. For example, with the Standard profile I can visit /admin/structure/types/manage/page/fields/node.page.body, and I see the page title "Text_with_summary settings for Basic page". I think we should use the field name when it is available.

I also left some comments on the MR. We need the usual BC steps when adding a parameter to the constructor, and that means we need a change record. I am adding the issue tag for that.

xjm → credited benjifisher → .

I don't know if it's OK, since it is an old unmaintained library?

Right, I prefer not to touch it unless it is doing something obviously wrong.

If we are going to change it, then we should add some tests to make sure that we are not getting some undesired behavior. Can you provide some test cases: strings before and after applying the typogrify filter? Better yet, could you add them to the data provider in the automated test?

I am not sure that spaceToNbsp() is the right place to make this change. It does not match the purpose of that function, as described in its doc block.

Looking at the code (but not testing) it seems that the change to i18nQuotes() will insert the non-breaking space in some cases that currently do not get any sort of space. That seems like an unwanted side effect.

benjifisher → made their first commit to this issue’s fork.

I am adding 📌 Fix the issues reported by phpcs RTBC as a related issue. While testing that issue, I noticed some PHP 8.2 deprecation notices:

Deprecated function: Use of "self" in callables is deprecated in Drupal\typogrify\Typogrify::caps() (line 110 of modules/typogrify/src/Typogrify.php).

Since Drupal 11 requires PHP 8.3, I think we should fix that as part of this issue.

@nayana_mvr: Thanks for updating the MR.

I rebased the MR on the 8.x-1.x branch. Thanks to 📌 Move automated tests to GitLab CI Fixed , that runs phpcs as a CI job.

I added a commit to sort the use statements, which is required by phpcs.

I reviewed the changes: they look correct to me. The CI job for phpcs passes.

I tested with Drupal 10.2.x, and basic functionality is working. However, I see this deprecation notice:

Deprecated function: Use of "self" in callables is deprecated in Drupal\typogrify\Typogrify::caps() (line 110 of modules/typogrify/src/Typogrify.php).

According to https://php.watch/versions/8.2/partially-supported-callable-deprecation, that pattern is deprecated in PHP 8.2. I think that is out of scope for this issue. Since Drupal 11 requires PHP 8.3, it should be fixed as part of 📌 Automated Drupal 11 compatibility fixes for typogrify Postponed .

I rebased the MR on the 8.1.x branch so that the tests run on CI (thanks to 📌 Move automated tests to GitLab CI Fixed ). The tests all passed: so far, so good.

Then I tested manually with the latest 11.x, and noticed some errors when I enable the filter. I tested with 10.3.x and saw the same errors, but I did not see a problem with 10.2.x.

I opened 🐛 Typogrify does not validate its settings Needs review to fix the errors, and I am postponing this issue on that one.

Four of the settings are serialized strings. This is left over from the original port of this module from Drupal 7 to Drupal 8. I added 📌 Do not serialize settings Active to handle the settings without serializing them.

benjifisher → created an issue.

benjifisher → created an issue.

benjifisher → made their first commit to this issue’s fork.

P.S. Thanks for linking to 🐛 Update composer metapackages Active , @tobiasb

Short version: I think we can close this issue as a duplicate of 🐛 Update composer metapackages Active .

The only thing I still question is why the failing tests did not show up in automated tests when 8ddf9986e0 was committed. There was a failure on a MR I added to the private fork used by the security team.

From #6:

What fails currently without this?

In the issue summary, I mentioned two tests.

Details:

At the time I created this issue, the HEAD of the 11.x branch was 8ddf9986e0. The commit for #3445224 is d7235bfe2f.

$ git log -1
commit 8ddf9986e0ab6f59d8b77d7545a629f104d6b5fe (HEAD -> 11.x, drupal-3445211/11.x)
Author: catch <catch@35733.no-reply.drupal.org>
Date:   Fri May 3 22:08:05 2024 +0100

    Back to dev.
$ ddev exec phpunit -c core core/tests/Drupal/Tests/Composer/Generator/MetapackageUpdateTest.php
PHPUnit 9.6.19 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Composer\Generator\MetapackageUpdateTest
F.F                                                                 3 / 3 (100%)

Time: 00:00.026, Memory: 6.00 MB

There were 2 failures:

1) Drupal\Tests\Composer\Generator\MetapackageUpdateTest::testUpdated with data set #0 ('Drupal\Composer\Generator\Bui...uilder', 'composer/Metapackage/CoreRecommended')
The rebuilt version of composer/Metapackage/CoreRecommended does not match what is in the source tree.

To fix, run:

    COMPOSER_ROOT_VERSION=11.0.x-dev composer update --lock

Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
         "webflo/drupal-core-strict": "*"\n
     },\n
     "require": {\n
-        "drupal/core": "11.0.x-dev",\n
+        "drupal/core": "11.x-dev",\n
         "asm89/stack-cors": "~v2.2.0",\n
         "composer/semver": "~3.4.0",\n
         "doctrine/annotations": "~2.0.1",\n

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:95
/var/www/html/core/tests/Drupal/Tests/Composer/Generator/MetapackageUpdateTest.php:84
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:729
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:685
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:685
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:146
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:99

2) Drupal\Tests\Composer\Generator\MetapackageUpdateTest::testUpdated with data set #2 ('Drupal\Composer\Generator\Bui...uilder', 'composer/Metapackage/PinnedDe...encies')
The rebuilt version of composer/Metapackage/PinnedDevDependencies does not match what is in the source tree.

To fix, run:

    COMPOSER_ROOT_VERSION=11.0.x-dev composer update --lock

Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
         "webflo/drupal-core-require-dev": "*"\n
     },\n
     "require": {\n
-        "drupal/core": "11.0.x-dev",\n
+        "drupal/core": "11.x-dev",\n
         "behat/mink": "v1.11.0",\n
         "behat/mink-browserkit-driver": "v2.2.0",\n
         "colinodell/psr-testlogger": "v1.3.0",\n

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:95
/var/www/html/core/tests/Drupal/Tests/Composer/Generator/MetapackageUpdateTest.php:84
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:729
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:685
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestSuite.php:685
/var/www/html/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:146
/var/www/html/vendor/phpunit/phpunit/src/TextUI/Command.php:99

FAILURES!
Tests: 3, Assertions: 3, Failures: 2.
Failed to execute command phpunit -c core core/tests/Drupal/Tests/Composer/Generator/MetapackageUpdateTest.php: exit status 1

After checking out the later commit,

$ git log -1
commit d7235bfe2f5fb0432aa931d620c09cea2b246e3f (HEAD)
Author: catch <catch@35733.no-reply.drupal.org>
Date:   Sat May 4 11:03:00 2024 +0100

    Issue #3445224 by catch, Gábor Hojtsy: Update composer metapackages
$ ddev exec phpunit -c core core/tests/Drupal/Tests/Composer/Generator/MetapackageUpdateTest.php
PHPUnit 9.6.19 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\Composer\Generator\MetapackageUpdateTest
...                                                                 3 / 3 (100%)

Time: 00:00.021, Memory: 6.00 MB

OK (3 tests, 3 assertions)

benjifisher → created an issue.

We discussed this issue at 📌 Drupal Usability Meeting 2024-05-03 Needs work . That issue will have a link to a recording of the meeting.

For the record, the attendees at the usability meeting were @benjifisher, @markusVJH, @matthieuscarset, @rkoller, @shaal, @simohell, and @skaught.

We considered what small steps we might make to move this issue forward. We came up with

• The original suggestion when this issue was created. (See the "Original report" section of the issue summary.) We agreed to add a new child issue for that.
• #3249370: Move the active Default and Admin Theme to a new section on top of the Appearance page →
• Improve the status message when selecting a default theme. (Issue needed.) The current message is something like this: "Note that the administration theme is still set to the Claro theme; consequently, the theme on this page remains unchanged. All non-administrative sections of the site, however, will show the selected Umami theme by default."

We also asked what the biggest usability problem is with the current page. We might come up with a different answer the next time we ask that question, but today we decided that choosing a default theme and choosing an admin theme should be more consistent. That should be addressed on #3249379: Make all form submissions on the Appearance page consistent → .

Several of the child issues are tagged as Needs design → . One way to move these issues forward is to implement something and then ask for feedback on the design.

benjifisher → created an issue.

benjifisher → created an issue.

benjifisher → created an issue.

oops

benjifisher → created an issue.

benjifisher → created an issue.

benjifisher → credited benjifisher → .

The first test failure I looked at is in ConfigureApplicationFormTest. It looks as though ConfigureApplicationForm::submitForm() calls Subscription::getSubscription(), which calls Subscription::hasCredentials(), and that function uses $this->settings directly instead of getSettings().

I think all references to $this->settings should be replaced with $this->getSettings(). (No, not the reference inside populateSettings().) That might fix a lot of the test failures.

I am adding more detail to the proposed resolution and replacing links to issues with tokens.

@danflanagan8:

Thanks for taking a look!

I added some tests, starting with yours from ✨ Add wrapper process plugin to wrap/unwrap values in arrays Needs review . That way,

1. Your excellent test coverage does not go to waste if we decide to close that issue in favor of this one.
2. I confirm that this plugin can do anything the wrap plugin can do (with method: wrap).

When I first wrote this plugin, I called it array_build, but then I realized the problem. I confess I was not very creative when I turned it into build_array, and you are right about the potential for confusion.

I can go with array_template, but other variations are worth considering:

• array_template
• template_array
• template

Or maybe the name should indicate that it can work with source, destination, and pipeline values.

How do you feel about using the short form "dest" instead of spelling out "destination"?

I updated some of the formatting and added issue credits. Someone else should review the credits.

I am crediting @ddavisboxleitner for adding the transcript.

Usability review

We discussed this issue at 📌 Drupal Usability Meeting 2024-04-19 Needs work . That issue will have a link to a recording of the meeting.

For the record, the attendees at the usability meeting were @AaronMcHale, @benjifisher, @rkoller, @shaal, @simohell, @skaught, and @worldlinemine.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

There was some discussion about whether we should handle multiple email addresses in separate textfields instead of allowing a comma-separated list. We decided that the current approach is the right choice. It is consistent with the email specification, and the Field API (not the Form API) is the right place to handle multiple textfields.

We tested various invalid entries, and we used the Disable HTML5 validation → module so that we could see the error messages provided by this issue. The current approach works well, but we would like to make two changes:

1. Do not treat empty emails differently from other invalid emails.
2. Insert a comma in the second sentence: "Use the format user@example.com, and separate the addresses with a comma."

An error message should do two things: describe the error state and give information on how to fix it. The current message, "All email addresses must be non-empty.", tries to do both at once. We think it would be clearer to use the same format as the other error messages:

The email address "" is not valid. Use the format user@example.com, and separate the addresses with a comma.

If possible, use the un-trimmed value in the error message so that there is a distinction between ,, and , ,.

Other argument in favor of (1):

• The error messages are more consistent.
• If a user enters an empt address and an invalid one, then currently they get the message about an empty address. They fix it and try again, and get the message about the invalid address. Let's list all the errors at once.

The second point is purely grammatical: when combining independent clauses with "and", there should be a comma after the first one.

benjifisher → created an issue.

benjifisher → created an issue.

I think you can get the same effect using the build_array process plugin from ✨ Process plugin: build an array from source, destination, pipeline Needs work .

Instead of this example from the doc block for iterate

process:
  field_dates:
    plugin: iterate
    source: field_timestamps
    process:
      plugin: format_date
      from_format: 'U'
      to_format: 'Y-m-d\TH:i:s'

you can use build_array to prepare the array of arrays that sub_process expects:

process:
  field_dates:
    - plugin: build_array
      source: field_timestamps
      template:
        - 'pipeline:'
    - plugin: sub_process
      process:
        plugin: format_date
        from_format: 'U'
        to_format: 'Y-m-d\TH:i:s'
        source: '0'

As @danflanagan8 pointed out in #3, it is much simpler in this case to let the Migrate API loop through the array for you, but I think this example shows that any time you want to use iterate, you can get the same effect with build_array and sub_process.

Many simple uses of sub_process can also be handled by build_array. The second example from #3 becomes

field_node_references:
  -
    plugin: explode
    delimiter: '/'
    source: field_links
  -
    plugin: build_array
    template:
      - pipeline:2
  -
    plugin: migration_lookup
    migration: node

The build_array plugin uses array_walk_recursive(), which is a lot simpler than sub_process and iterate, both of which create a separate process pipeline. So I prefer to use build_array when it can replace one of the others.

Some more examples, from the doc block:

Generic example:

process:
  bar:
    plugin: build_array
    source: foo
    template:
      key: literal string
      properties:
        - source:field_body/0/value
        - dest:field_body/0/value
      - pipeline:some/nested/key

Prepare an entity reference revision (ERR) field

process:
  field_paragraph:
    - plugin: migration_lookup
      # ...
    - plugin: build_array
      template:
        target_id: pipeline:0
        target_revision_id: pipeline:1

Here is an example from my current project.

Create a serialized array for the layout_paragraphs module

process:
  behavior_settings:
    # ...
    - plugin: build_array
      template:
        layout_paragraphs:
          parent_uuid: pipeline:0/value
          region: first
    - plugin: callback
      callable: serialize

I think that wrapper with method: wrap can be replaced by the more flexible build_array plugin from ✨ Process plugin: build an array from source, destination, pipeline Needs work ; and method: unwrap can be replaced by callback with callable: array_pop.

For example, the pipeline from the issue description,

  -
    plugin: wrapper
    method: wrap
    source: my_array
    key: element
  -
    plugin: sub_process
    process:
      '0':
        plugin: flatten
        source: element # This should match the 'key' used when wrapping.
  -
    plugin: wrapper
    method: unwrap

is equivalent to

  - 
    plugin: build_array
    template:
      element: 'pipeline:'
    source: my_array
  -
    plugin: sub_process
    process:
      '0':
        plugin: flatten
        source: element
  - plugin: callback
    callable: array_pop

I verified that they give the same results on the example input using migrate_sandbox.

So I suggest we close this issue in favor of ✨ Process plugin: build an array from source, destination, pipeline Needs work .

In #3236774-9: Provide ability to reference current value of process pipeline as a source property → , @danflanagan8 pointed out that passing null as part of the source array of a process plugin has the effect of inserting the pipeline value.

At 📌 [meeting] Migrate Meeting 2024-03-29 Active , @mikelutz said that this behavior is a bug, and we should avoid using it.

The process plugin proposed here is more flexible, and it makes the intention clearer.

I think this process plugin is also more flexible than the one proposed in ✨ Add wrapper process plugin to wrap/unwrap values in arrays Needs review .

benjifisher → created an issue.

I see that @skaught commented on ✨ Enable/Disable View within View Edit page Needs review and removed the "Needs usability review" issue tag, so I think all the tasks for this meeting are complete.

benjifisher → created an issue.

benjifisher → created an issue.

@smethawee:

Did you clear caches after applying the patch?

Do you have any custom or contrib modules that define layouts using PHP classes? The patch fixes core/modules/layout_builder/src/Plugin/Layout/BlankLayout.php, but you might have files like mymodule/src/Plugin/Layout/MyLayout.php with the same problem.

benjifisher → created an issue.

benjifisher → created an issue.

benjifisher → created an issue.