🇨🇭Switzerland @znerol

Please do not use this patch. It is not going to land. Instead see the official docs on how to use image replace with core media → .

ACK

Thanks @berdir for taking the time to write down your thoughts. I very much agree, I do not have anything to add.

Thank you!

znerol → created an issue.

Thanks @heddn, fixed.

Another follow-up: 📌 Inject ModuleExtensionList into EmbedTypeBase constructor Active

znerol → created an issue.

znerol → changed the visibility of the branch 3430097-automated-drupal-11 to hidden.

Automated tests are green for MR !25 . I did some manual testing with entity_embed and entity_browser. Haven't come across any problems so far. Also filed 📌 Add test coverage for EmbedSettingsForm Active as another follow-up.

znerol → created an issue.

There are still legit unresolved comments in MR !24. Going through the history of this issue, I notice that there has been a release with a BC break which had to be reverted subsequently (#50). I am not surprised that the maintainers are reluctant to merge this as is.

It was suggested to keep changes to a minimum when adapting contrib modules to new core releases. I very much agree with that approach. Based on the recommendation in #53 I'm going to work on a new MR with only the changes required for a D11 compatible release. The rest should go into dedicated issues:

• 💬 Drop support of CKEditor4 and include functionjavascript tests for ckeditor5 Needs work
• 🐛 PHPStan issue reported in gitlab Active
• 🐛 Fix PhpCS issue reported in gitlab RTBC
• 📌 Fix ESLint pipeline Active

+1 for removal.

!10622 Draft regrettably doesn't fail in a useful way.

znerol → created an issue.

I think I have a lead. Since 📌 OOP hooks using event dispatcher Needs review hook implementations are stored in a container parameter. That parameter is dumped along with the container when the very first request is handled by the standard front controller (index.php) and DrupalKernel.

Normally the very first request taking this code path is the redirect after the installation. An installation will complete successfully if that is the case.

Any stray request handled by the default front controller and DrupalKernel during installation will trigger a container build / dump. I guess that this might have lead to problems before. But now that HookCollectorPass sets the hook_implementations_map parameter which is subsequently used by ModuleHandler when hooks are invoked, the effect of an outdated container loaded from cache is much worse.

In order to isolate its environment, the installer uses a special InstallerKernel with container dumping disabled. The final drupal_cms_installer_uninstall_myself step is executed in the installer environment as well. It actually does trigger a container rebuild, but the resulting container is not dumped. After the subsequent redirect, a stale container is loaded from cache and ModuleHandler uses an outdated hook implementations map where drupal_cms_installer hook implementations are still present.

I can produce that stack trace by just curling any random invalid path during the install. E.g.:

% curl http://localhost:8888/bla
The website encountered an unexpected error. Try again later.<br><br><em class="placeholder">Error</em>: Call to a member function getPath() on null in <em class="placeholder">drupal_cms_installer_theme_registry_alter()</em> (line <em class="placeholder">295</em> of <em class="placeholder">profiles/drupal_cms_installer/drupal_cms_installer.profile</em>). <pre class="backtrace">Drupal\Core\Extension\ModuleHandler-&gt;alter() (Line: 434)
Drupal\Core\Theme\Registry-&gt;build() (Line: 276)
Drupal\Core\Theme\Registry-&gt;get() (Line: 88)
Drupal\Core\Utility\ThemeRegistry-&gt;initializeRegistry() (Line: 69)
Drupal\Core\Utility\ThemeRegistry-&gt;__construct() (Line: 314)
Drupal\Core\Theme\Registry-&gt;getRuntime() (Line: 141)
Drupal\Core\Theme\ThemeManager-&gt;render() (Line: 446)
Drupal\Core\Render\Renderer-&gt;doRender() (Line: 203)
Drupal\Core\Render\Renderer-&gt;render() (Line: 158)

I also got the trace when the installer completes in Firefox. But my machine is really old and really slow, so that might help with the race condition.

[12606] [Thu Dec 19 07:45:36 2024] [::1]:54554 [302]: GET /core/install.php?profile=drupal_cms_installer&langcode=en&recipes%5B0%5D=drupal_cms_starter&recipes%5B1%5D=drupal_cms_blog&recipes%5B2%5D=drupal_cms_case_study&recipes%5B3%5D=drupal_cms_events&recipes%5B4%5D=drupal_cms_news&recipes%5B5%5D=drupal_cms_person&recipes%5B6%5D=drupal_cms_project&site_name=My%20Drupal%20CMS%20site
[12606] [Thu Dec 19 07:45:36 2024] [::1]:54554 Closing
[12607] [Thu Dec 19 07:45:36 2024] [::1]:36944 Accepted
[12607] [Thu Dec 19 07:45:36 2024] [::1]:36944 [302]: GET //admin/dashboard/welcome
[12607] [Thu Dec 19 07:45:36 2024] [::1]:36944 Closing
[12593] [Thu Dec 19 07:45:36 2024] [::1]:36958 Accepted
[12593] [Thu Dec 19 07:45:36 2024] Uncaught PHP Exception Error: "Call to a member function getPath() on null" at /[...]/web/profiles/drupal_cms_installer/drupal_cms_installer.profile line 295
[12593] [Thu Dec 19 07:45:36 2024] [::1]:36958 [500]: GET /admin/dashboard/welcome?check_logged_in=1
[12593] [Thu Dec 19 07:45:36 2024] [::1]:36958 Closing
[12605] [Thu Dec 19 07:45:37 2024] [::1]:36962 Accepted
[12605] [Thu Dec 19 07:45:37 2024] [::1]:36962 [404]: GET /favicon.ico - No such file or directory
[12605] [Thu Dec 19 07:45:37 2024] [::1]:36962 Closing

True. But the function is named like this: views_core_entity_reference_update_install.

The module name is: views_core_entity_reference. Hence, module handler will interpret the remaining _update_install as hook_update_N instead of hook_install.

This MR introduced the following chunk in views_core_entity_reference.install:

/**
 * Implements hook_install().
 */
function views_core_entity_reference_update_install($is_syncing) {
  _views_core_entity_reference_update_as_a_reference();
}

If I'm not mistaken, the new hook_install() implementation is in fact a hook_update_N implementation (where N = install instead of a number).

Deprecation message updated. Good candidates for reviewers are the maintainers of the mailsystem → module I guess.

Thanks @prem suthar, the MR looks right. Regrettably, there are two test failures in @core/modules/user/tests/src/Unit/Plugin/Core/Entity/UserTest.php@. I can reproduce them locally. The full output:

% ../vendor/bin/phpunit modules/user/tests/src/Unit/Plugin/Core/Entity/UserTest.php
PHPUnit 10.5.38 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.14
Configuration: /home/lo/src/drupal/core/phpunit.xml

.EE                                                                 3 / 3 (100%)

Time: 00:00.199, Memory: 16.00 MB

There were 2 errors:

1) Drupal\Tests\user\Unit\Plugin\Core\Entity\UserTest::testHasRole
InvalidArgumentException: The entity object refers to a removed translation (x-default) and cannot be manipulated.

/home/lo/src/drupal/core/lib/Drupal/Core/Entity/ContentEntityBase.php:609
/home/lo/src/drupal/core/lib/Drupal/Core/Entity/ContentEntityBase.php:597
/home/lo/src/drupal/core/modules/user/src/Entity/User.php:193
/home/lo/src/drupal/core/modules/user/src/Entity/User.php:206
/home/lo/src/drupal/core/tests/Drupal/Tests/Core/Session/UserSessionTest.php:77
/home/lo/src/drupal/vendor/bin/phpunit:122

2) Drupal\Tests\user\Unit\Plugin\Core\Entity\UserTest::testUserGetRoles
InvalidArgumentException: The entity object refers to a removed translation (x-default) and cannot be manipulated.

/home/lo/src/drupal/core/lib/Drupal/Core/Entity/ContentEntityBase.php:609
/home/lo/src/drupal/core/lib/Drupal/Core/Entity/ContentEntityBase.php:597
/home/lo/src/drupal/core/modules/user/src/Entity/User.php:193
/home/lo/src/drupal/core/modules/user/tests/src/Unit/Plugin/Core/Entity/UserTest.php:56
/home/lo/src/drupal/vendor/bin/phpunit:122

--

2 tests triggered 2 PHP warnings:

1) /home/lo/src/drupal/core/lib/Drupal/Core/Entity/ContentEntityBase.php:608
Undefined array key "x-default"

Triggered by:

* Drupal\Tests\user\Unit\Plugin\Core\Entity\UserTest::testHasRole
  /home/lo/src/drupal/core/tests/Drupal/Tests/Core/Session/UserSessionTest.php:71

* Drupal\Tests\user\Unit\Plugin\Core\Entity\UserTest::testUserGetRoles
  /home/lo/src/drupal/core/modules/user/tests/src/Unit/Plugin/Core/Entity/UserTest.php:53

2) /home/lo/src/drupal/core/lib/Drupal/Core/Entity/ContentEntityBase.php:608
Trying to access array offset on null

Triggered by:

* Drupal\Tests\user\Unit\Plugin\Core\Entity\UserTest::testHasRole
  /home/lo/src/drupal/core/tests/Drupal/Tests/Core/Session/UserSessionTest.php:71

* Drupal\Tests\user\Unit\Plugin\Core\Entity\UserTest::testUserGetRoles
  /home/lo/src/drupal/core/modules/user/tests/src/Unit/Plugin/Core/Entity/UserTest.php:53

ERRORS!
Tests: 3, Assertions: 2, Errors: 2, Warnings: 2.

Not sure, maybe it is necessary to modify User::getRoles() and use $this->getFieldValue('roles') instead of $this->get('roles'). Possibly @berdir could help here.

svendecabooter → credited znerol → .

znerol → created an issue.

Great! Thanks @budalokko

I hope this company will eventually give up spamming contrib projects with identical issues. My recommendation to the maintainers: consequently close issues from those people with status works as designed.

Change recommendations on the webhook listener: 1. Use the fullfill state in accordance to the pf docs, enable payload signing.

znerol → created an issue.

Great work. Tests are green. Verified manually that no invalid session ids are created in the DB. Thanks a lot @kentr.

Thanks, left some remarks in the MR. 📌 Use session.lazy-write once Drupal 8 only supports PHP 7 Active should be trivial after this landed.

I do prefer using events in many situations for the exact same reason other people dislike them: Introducing an event requires introducing a custom value object class. Introducing a hook doesn't.

For code authors, the event class is a good place to document requirements and expectations for an extension point. I know there is mymodule.api.php but in my personal experience, I cannot be trusted to keep that up-to-date. Even more so because I do not have any tooling for my IDE to display hook documentation where it would be most useful (i.e. when I implement hooks). With the event pattern, the docs are available where the event is used.

I think the event value object class having its own distinct type reduces accidents due to type confusion and it simplifies static code analysis. But I guess that hook implementations could be just as safe with a set of phpstan rules examining hook implementation parameters.

For code users, the event class is handy to quickly navigate to places where an event is dispatched or where it is handled. Sure, I can grep for Implements hook_XYZ (or nowadays #Hook('XYZ')) and just hope that I find every implementation. But I do enjoy the fact that my IDE is able to quickly list every usage of an event class with built-in tooling. No magic needed at all.

My gut feeling is that hooks have their strengths when operating on loosely typed stuff (e.g., form structures, render arrays, processing of twig variables, etc.). On the other hand, events are best used in conjunction with clearly defined types (e.g., UserFloodEvent operating on a user entity in core or the commerce_order.order.paid event operating on an order in commerce).

This needs a reroll from #152 , and it should be converted to a merge request.

Addressed #24 in a slightly different way. Let's just follow a core example (e.g. ContactFormListBuilder).

The remaining test fail is a bit tricky:

Drupal\Tests\commerce_reports\Kernel\ReportType\OrderReportTest::testOrderReport
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
     'given_name' => 'Frederick'
     'additional_name' => null
     'family_name' => 'Pabst'
+    'address_line3' => null
 )

/builds/issue/commerce_reports-3428485/tests/src/Kernel/ReportType/OrderReportTest.php:129

I guess this is due to address issue ✨ Add address line 3 Fixed . I think the actual and expected values should just run through array_filter() before comparing them. It looks like commerce hides address_line3 by default, but order reports doesn't. Which is fine IMHO.

I'm looking at the test failures, first concentrating on kernel test. The failures in OrderReportGeneratorTest are mildly concerning. Given the following stack trace:

Drupal\Tests\commerce_reports\Kernel\OrderReportGeneratorTest::testGenerateReports
Error: Call to a member function getPattern() on null

/builds/issue/commerce_reports-3428485/web/core/lib/Drupal/Core/Datetime/DateFormatter.php:133
vfs://root/sites/simpletest/94104550/files/php/twig/674dca7cb14c7_commerce-order-receipt.ht_3Oi4ZV8pyQw7FpmRkQSEdRWRT:90
/builds/issue/commerce_reports-3428485/vendor/twig/twig/src/Template.php:393
/builds/issue/commerce_reports-3428485/vendor/twig/twig/src/Template.php:349
/builds/issue/commerce_reports-3428485/vendor/twig/twig/src/Template.php:364
/builds/issue/commerce_reports-3428485/vendor/twig/twig/src/TemplateWrapper.php:35
/builds/issue/commerce_reports-3428485/web/core/themes/engines/twig/twig.engine:33
/builds/issue/commerce_reports-3428485/web/core/lib/Drupal/Core/Theme/ThemeManager.php:348
/builds/issue/commerce_reports-3428485/web/core/lib/Drupal/Core/Render/Renderer.php:446
/builds/issue/commerce_reports-3428485/web/core/lib/Drupal/Core/Render/Renderer.php:203
/builds/issue/commerce_reports-3428485/web/core/lib/Drupal/Core/Render/Renderer.php:120
/builds/issue/commerce_reports-3428485/web/core/lib/Drupal/Core/Render/Renderer.php:593
/builds/issue/commerce_reports-3428485/web/core/lib/Drupal/Core/Render/Renderer.php:119
/builds/issue/commerce_reports-3428485/web/modules/contrib/commerce/commerce.module:73
/builds/issue/commerce_reports-3428485/web/core/lib/Drupal/Core/Extension/ModuleHandler.php:407
/builds/issue/commerce_reports-3428485/web/core/lib/Drupal/Core/Mail/MailManager.php:273
/builds/issue/commerce_reports-3428485/web/core/lib/Drupal/Core/Mail/MailManager.php:181
/builds/issue/commerce_reports-3428485/web/core/lib/Drupal/Core/Render/Renderer.php:593
/builds/issue/commerce_reports-3428485/web/core/lib/Drupal/Core/Mail/MailManager.php:180
/builds/issue/commerce_reports-3428485/web/modules/contrib/commerce/src/MailHandler.php:105
/builds/issue/commerce_reports-3428485/web/modules/contrib/commerce/modules/order/src/Mail/OrderReceiptMail.php:76
/builds/issue/commerce_reports-3428485/web/modules/contrib/commerce/modules/order/src/EventSubscriber/OrderReceiptSubscriber.php:63
/builds/issue/commerce_reports-3428485/vendor/symfony/event-dispatcher/EventDispatcher.php:246
/builds/issue/commerce_reports-3428485/vendor/symfony/event-dispatcher/EventDispatcher.php:206
/builds/issue/commerce_reports-3428485/vendor/symfony/event-dispatcher/EventDispatcher.php:56
/builds/issue/commerce_reports-3428485/web/modules/contrib/state_machine/src/Plugin/Field/FieldType/StateItem.php:425
/builds/issue/commerce_reports-3428485/web/modules/contrib/state_machine/src/Plugin/Field/FieldType/StateItem.php:392
/builds/issue/commerce_reports-3428485/web/core/lib/Drupal/Core/Field/FieldItemList.php:233
/builds/issue/commerce_reports-3428485/web/core/lib/Drupal/Core/Field/FieldItemList.php:198
/builds/issue/commerce_reports-3428485/web/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:938
/builds/issue/commerce_reports-3428485/web/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:979
/builds/issue/commerce_reports-3428485/web/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:896
/builds/issue/commerce_reports-3428485/web/modules/contrib/commerce/src/CommerceContentEntityStorage.php:56
/builds/issue/commerce_reports-3428485/web/modules/contrib/commerce/modules/order/src/OrderStorage.php:89
/builds/issue/commerce_reports-3428485/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php:564
/builds/issue/commerce_reports-3428485/web/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php:781
/builds/issue/commerce_reports-3428485/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php:489
/builds/issue/commerce_reports-3428485/web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:805
/builds/issue/commerce_reports-3428485/web/modules/contrib/commerce/modules/order/src/OrderStorage.php:169
/builds/issue/commerce_reports-3428485/web/core/lib/Drupal/Core/Entity/EntityBase.php:354
/builds/issue/commerce_reports-3428485/tests/src/Kernel/OrderReportGeneratorTest.php:140

The failure is due to a failure while rendering the commerce-order-receipt.html.twig template. The offending line is where the order date is rendered:

              <div style="margin-top: 5px;"><span style="font-weight: bold;">{{ 'Order date:'|t }}</span> {{ order_entity.getPlacedTime|format_date('short') }}</div>

In short, the test setup is missing the core.date_format.short config entity. I am able to fix the error locally by inserting $this->installConfig(['system']); into the setUp() method. On the other hand, it would probably be better to disable order receipts in the first place while testing. Going to try that next.

Added a CR draft.

Fantastic work so far. Thanks a lot @kentr. While thoroughly reviewing existing tests I found one thing which indicates lack of coverage:

The MR adds new methods to TestSessionHandlerProxy.php. That class is used to verify correct propagation of session handler calls throughout the handler stack.

However, those new methods seemingly do not have any effect on the test which is asserting on the results of TestSessionHandlerProxy, namely StackSessionHandlerIntegrationTest.

Looking at the test, I think it only covers a situation where a new session is opened. But it doesn't cover the situation where the new methods are called from within the php session subsystem. I.e., it doesn't cover the case when an existing session is used (valid session cookie exists on the request). E.g., on a subsequent read (without any write to the session), I'd expect a trace including validateId before read and updateTimestamp instead of write.

In order to produce such a trace, it would be necessary to extend the SessionTestController.php with a method similar to traceHandler(), maybe asserting that $_SESSION['trace-handler'] is non-zero but without changing it. The StackSessionHandlerIntegrationTest::testRequest then could subsequently call into that and assert that the new methods on TestSessionHandlerProxy are actually invoked.

Tests passed, including the one which failed before:

Drupal\Tests\block\Functional\BlockTest                       14 passes   80s

There is one test failure:

Drupal\Tests\block\Functional\BlockTest::testBlockAccess
Behat\Mink\Exception\ResponseTextException: The text "Hello test world" was not found anywhere in the text of the current page.

vendor/behat/mink/src/WebAssert.php:907
vendor/behat/mink/src/WebAssert.php:293
core/tests/Drupal/Tests/WebAssert.php:979
core/modules/block/tests/src/Functional/BlockTest.php:581

I've seen that failing randomly in other MRs. Triggered a rerun in order to check whether this is reproducible.

With the test coverage I'm pretty confident that everything is working as expected now. Need to think about the docs, it is somewhat important that people first enable payload signing at the PSP side and only afterwards the webhook access check.

I pushed it so that others can observe, but I believe updateTimestamp() should return TRUE to be seen as a successful no-op.

Yes, you are right. A no-op implementation of updateTimestamp() needs to return TRUE.

I've been researching this a little bit. We have the following mechanism in core already:

1. Drupal core has the session_write_interval setting which defaults to 180 seconds.
2. That setting is used in core MetadataBag to initialize its parent.
3. The Symfony MetadataBag will update its timestamp when the update threshold is reached.
4. When that happens, session metadata is changed and SessionHandler:write() is called when the request terminates.
5. That in turn will also update the timestamp field.

Hence, in order to avoid unnecessary writes, the updateTimestamp() method must not touch the database. Instead it should just return FALSE;.

Test result of the failing test:

Drupal\Tests\system\Functional\Session\SessionTest::testSessionWrite
Sessions table was not updated.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'1733600778'
+'1733600779'

core/modules/system/tests/src/Functional/Session/SessionTest.php:346

This is in testSessionWrite().

My hunch is that now that SessionHandler implements updateTimestamp(), the testSessionWrite() fails because session data is now updated where it wasn't before.

Many repeated writes into the session table when no update happened could lead to spiking db traffic on existing sites. So I suggest to just add a stub in this issue and asses whether / how we want to implement updateTimestamp() in a follow-up. Maybe we need logic to throttle updates to the timestamp column.

No need to change that at the moment.

Thanks @kentr for picking this up (and thanks @neclimdul for the initial work). In the mean time [#15222090] and 📌 Add return types to SessionHandler Active landed, so we do not have any typing related BC problems here anymore.

I'll stick around for reviews, let's land this.

Implement this as an access check. This needs tests and the check needs to be configurable in some way in order to provide a smooth transition.

There WebhookEncryptionService::isContentValid which does all the parsing, but also the read() call. Since I'm not keen to reimplement that, option 3 is out.

In order to get things rolling here, I'll implement for option 2 for the start.

Regrettably the ApiClient constructor requires an user id and a secret.

Hence, we basically have three options to retrieve the public key when a webhook signature needs checking:

1. Stick with ApiClient and correct credentials. In order to do that we'd need a way to configure some sort of global or default credentials which are not bound to a payment method.
2. Stick with ApiClient and supply fake credentials. Easy to implement, but doesn't feel like a proper solution.
3. Use guzzle without credentials. This additionally opens up the option to cache the public key and avoid repeated lookups.

The Webhook Encryption Service > read endpoint is a bit different than other resources on this API:

1. It does not require authentication.
2. It does not take any spaceId parameter.

It follows that the keyId field in the x-signature request header in the webook call is neither bound to an account nor to a space. At the moment, webhook calls from all of my test accounts are signed with the same keyID=15a411ae-5d1b-465e-aaa0-65347e4c9f0e.

The public key can be retrieved using WebhookEncryptionService::read($id) - or any other HTTP client. Calling the endpoint with curl reveals that there are proper caching headers (cache-control: public, max-age=31536000) on the response.

% curl -ifs 'https://checkout.postfinance.ch/api/webhook-encryption/read?id=15a411ae-5d1b-465e-aaa0-65347e4c9f0e'
HTTP/2 200
date: Fri, 06 Dec 2024 13:16:33 GMT
content-type: application/json;charset=utf-8
vary: Accept-Encoding
x-svid: 0e518d183291a1bcc
set-cookie: _csrf_token_443=79hlv52gv6h8dnu7f4gq373r4s; Path=/; Expires=Sat, 7 Dec 2024 13:16:33 GMT; Max-Age=86400; Secure; HttpOnly
set-cookie: language=en-US; Path=/; Secure; HttpOnly
set-cookie: language=en-US; Path=/; Secure; HttpOnly
expires: Sat, 06 Dec 2025 13:16:33 GMT
reporting-endpoints: csp-endpoint="/csp-reports"
report-to: {   "group": "csp-endpoint",   "max_age": 10886400,   "endpoints": [     { "url": "/csp-reports" }   ] }
content-security-policy: default-src 'self'; child-src 'self'; connect-src 'self'; font-src 'self'; frame-src 'self'; img-src 'self'; manifest-src 'self'; media-src 'self'; object-src 'none'; script-src 'self' 'nonce-W/InUZhxHt2YcndKLu8Utw=='; style-src 'self' 'nonce-W/InUZhxHt2YcndKLu8Utw=='; worker-src 'self'; report-to csp-endpoint; report-uri /csp-reports;
x-xss-protection: 1
cache-control: public, max-age=31536000
cf-cache-status: DYNAMIC
strict-transport-security: max-age=0; includeSubDomains; preload
x-content-type-options: nosniff
server: cloudflare
cf-ray: 8edc8d165e9627bd-LYS

{"id":"15a411ae-5d1b-465e-aaa0-65347e4c9f0e","publicKey":"MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEcP72Buvf1myOG9qqpcx4wkdKXdYC+FTCqstYH+NzX0o4vg6X0PY80nXmcbHlmu7TBMEc55MCv4z/n4Xy4HqC3A=="}

Nice catch! In that case core_version_requirement should be set to ^10.3 || ^11 I guess?

Thanks! Good find, that's probably an issue when operating on a preexisting entity.

Current MR just removes the workaround and as a result fails locking the order during the payment update. It should retain the locking.

To expand a little bit on the explanation above. Drupal in fact allows you to opt out of caching on specific routes using the no_cache route option. An example of such a route is system.cron in system.routing.yml.

system.cron:
  path: '/cron/{key}'
  defaults:
    _controller: '\Drupal\system\CronController::run'
  options:
    no_cache: TRUE
  requirements:
    _access_system_cron: 'TRUE'

This is also a really good example for a reasonable exception to the HTTP idempotency rule.

I suggest to close this issue (works as designed).

I've been tracking down a bug that happen on a custom page that set some session variables but ended up being cached.

I guess this custom page violates HTTP idempotency. In short, a GET or HEAD request should not modify data on the server.

The page cache module assumes that developers respect this principle. I.e., it assumes that GET and HEAD requests do not modify data and as a consequence do not try to set session values.

In order to modify data (even if its only session data), you need to use POST.

I'm not too happy keeping an unbound amount of state inside the page cache middleware. The original approach in #3029373: PageCache::getCacheId() sometimes uses different cache IDs during get and set, causing unnecessary cache misses for some requests → would be compatible with n main requests (look at the patches before comment #13.

Pushed a potential fix. This is intentionally violating coding standards for the sake of reviewability. Should the approach prove to be feasible, the blocks inside the new closures need to be indented properly before merging.

I'm seeing this occasionally on a production system. In my case it seems related to ajax updates.

Affected URL: example.com/checkout/NNNNNN/order_information?_wrapper_format=drupal_ajax&ajax_form=1

[commerce_order] [error] <pre>Drupal\commerce_order\Exception\OrderVersionMismatchException: Attempted to save order NNNNNN with version 6. Current version is 7. in modules/contrib/commerce/modules/order/src/Entity/Order.php:719
Stack trace:
#0 core/lib/Drupal/Core/Entity/EntityStorageBase.php(528): Drupal\commerce_order\Entity\Order->preSave(Object(Drupal\commerce_order\OrderStorage))
#1 core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php(753): Drupal\Core\Entity\EntityStorageBase->doPreSave(Object(Drupal\commerce_order\Entity\Order))
#2 core/lib/Drupal/Core/Entity/EntityStorageBase.php(483): Drupal\Core\Entity\ContentEntityStorageBase->doPreSave(Object(Drupal\commerce_order\Entity\Order))
#3 core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php(806): Drupal\Core\Entity\EntityStorageBase->save(Object(Drupal\commerce_order\Entity\Order))
#4 modules/contrib/commerce/modules/order/src/OrderStorage.php(169): Drupal\Core\Entity\Sql\SqlContentEntityStorage->save(Object(Drupal\commerce_order\Entity\Order))
#5 core/lib/Drupal/Core/Entity/EntityBase.php(354): Drupal\commerce_order\OrderStorage->save(Object(Drupal\commerce_order\Entity\Order))
#6 modules/contrib/commerce_shipping/src/Plugin/Commerce/CheckoutPane/ShippingInformation.php(310): Drupal\Core\Entity\EntityBase->save()
#7 modules/custom/custom_retail_checkout/src/Plugin/Commerce/CheckoutPane/RetailShippingInformation.php(23): Drupal\commerce_shipping\Plugin\Commerce\CheckoutPane\ShippingInformation->buildPaneForm(Array, Object(Drupal\Core\Form\FormState), Array)
#8 modules/contrib/commerce/modules/checkout/src/Plugin/Commerce/CheckoutFlow/CheckoutFlowWithPanesBase.php(546): Drupal\custom_retail_checkout\Plugin\Commerce\CheckoutPane\RetailShippingInformation->buildPaneForm(Array, Object(Drupal\Core\Form\FormState), Array)
#9 [internal function]: Drupal\commerce_checkout\Plugin\Commerce\CheckoutFlow\CheckoutFlowWithPanesBase->buildForm(Array, Object(Drupal\Core\Form\FormState), 'order_informati...')
#10 core/lib/Drupal/Core/Form/FormBuilder.php(536): call_user_func_array(Array, Array)
#11 core/lib/Drupal/Core/Form/FormBuilder.php(375): Drupal\Core\Form\FormBuilder->retrieveForm('commerce_checko...', Object(Drupal\Core\Form\FormState))
#12 core/lib/Drupal/Core/Form/FormBuilder.php(633): Drupal\Core\Form\FormBuilder->rebuildForm('commerce_checko...', Object(Drupal\Core\Form\FormState), Array)
#13 core/lib/Drupal/Core/Form/FormBuilder.php(326): Drupal\Core\Form\FormBuilder->processForm('commerce_checko...', Array, Object(Drupal\Core\Form\FormState))
#14 core/lib/Drupal/Core/Form/FormBuilder.php(224): Drupal\Core\Form\FormBuilder->buildForm(Object(Drupal\commerce_checkout\Plugin\Commerce\CheckoutFlow\MultistepDefault), Object(Drupal\Core\Form\FormState))
#15 modules/contrib/commerce/modules/checkout/src/Controller/CheckoutController.php(143): Drupal\Core\Form\FormBuilder->getForm(Object(Drupal\commerce_checkout\Plugin\Commerce\CheckoutFlow\MultistepDefault), 'order_informati...')
#16 [internal function]: Drupal\commerce_checkout\Controller\CheckoutController->formPage(Object(Drupal\Core\Routing\RouteMatch))
#17 core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(123): call_user_func_array(Array, Array)
[...]

I guess the ShippingInformation should use OrderStorage::loadForUpdate() when it is attempting to update shipments on the order.

Also modified the payment gateway plugin to use an attribute instead of an annotation.

Verified manually that the webhookTransaction() is still working as expected using my postfinance checkout test account and hookdeck.

Leaving this on needs-review for a couple of days in order give other people a chance to verify themselves.

znerol → created an issue.

znerol → created an issue.

+++ b/src/Plugin/Commerce/PaymentGateway/PostFinanceCheckout.php
@@ -567,6 +567,9 @@ class PostFinanceCheckout extends OffsitePaymentGatewayBase implements PostFinan
+    if ($postfinance_state == 'FAILED') {
+      $commerce_sate = 'canceled';
+    }

I was attempting to add tests for this change. However, it turns out that canceled is not valid as a payment state. See the commerce docs for a list of valid states, as well as the commerce_payment.workflows.yml for possible transitions.

znerol → created an issue.

Hi! Thanks for the report and the patch, and sorry it took that long before somebody took a look.

I guess the $payment->completed field shouldn't be updated for failed payments. It looks like this is subsequently checked by the PaymentOrderUpdater::updateOrder() to decide whether to update the total_paid field whenever the order is saved. Based on that, OrderStorage::doOrderPreSave() will dispatch the ORDER_PAID event.

znerol → created an issue.

znerol → created an issue.

znerol → created an issue.

znerol → created an issue.

znerol → created an issue.

znerol → created an issue.

znerol → created an issue.