Account created on 8 June 2006, almost 19 years ago
#

Merge Requests

More

Recent comments

🇨🇭Switzerland znerol

I assume retry_period from the changelog is not relevant to us?

No. Its injected into RoundRobinTransport if the DSN is parsed from a string. Since we are using Use structured DSN instead of URI in system.mail mailer_dsn Fixed , this isn't in the code path.

🇨🇭Switzerland znerol

Thanks, I like all of the suggestions.

I'm also wondering if the mail theme classes shouldn't be in the `Drupal\Core\Mail` namespace instead of `Drupal\Core\MailTheme`?

My plan was to deprecated everything in Drupal\Core\Mail eventually. 📌 [PP-1] Add symfony mailer transports to DIC Postponed introduces Drupal\Core\Mailer namespace as a replacement.

🇨🇭Switzerland znerol

Thanks @larowlan for the review.

the main concern from an FM POV is the number of transports added that may be redundant.

.

The MR adds one transport to the container (i.e., one instance of Symfony\Component\Mailer\Transport\TransportInterface). But in order to be able to instantiate that transport, the presence of N transport factories is necessary. Note that AutowireIterator iterates through the list of service definitions tagged with mailer.transport_factory and instantiates one-by-one until it finds one which is capable of creating the desired transport (defined by the DSN scheme).

Also, it turned out that the transport factories can be market non-public, yay! The only two public services added by the MR are now Symfony\Component\Mailer\Transport\TransportInterface and Symfony\Component\Mailer\MailerInterface.

🇨🇭Switzerland znerol

I wasn't happy how GitLab makes it difficult to access the recent failures data through the web UI. So, I took a shot at extracting these numbers from the API.

There is still lots in the data which isn't shown in the PoC dashboard. Please use the issue tracker of the linked projects or hit me up on slack if you have suggestions on how to improve it.

🇨🇭Switzerland znerol

@naidim this is just declaring a library dependency, not a module dependency. You still can use the Field Group module in production without enabling Field UI.

🇨🇭Switzerland znerol

Repeat test MR is passing. However, there is a fail in EntityReferenceWidgetTest::testWidgetPreview(). Is that an instance of a known random test fail?

https://git.drupalcode.org/issue/drupal-3514699/-/pipelines/476020/test_...

🇨🇭Switzerland znerol

According to #65 this is fixed in PHP. Closing.

🇨🇭Switzerland znerol

I agree with #6, issue #2921123: Adjust Rectangle class to calculate rotated image dimensions according to libgd 2.2.2+ contains valuable research leading to the fix. However, I think we can trust people to use git blame to actually find that research (via this issue). I removed the @see reference.

🇨🇭Switzerland znerol

Rerolled #40 and migrated to PR, still needs work for #48/#49.

🇨🇭Switzerland znerol

znerol made their first commit to this issue’s fork.

🇨🇭Switzerland znerol

Debugging FunctionalJavascript tests is f*** fun. I was trying numerous things to resolve a mysterious race condition yesterday. Looking at this with a fresh mind today, I discovered that saving the layout resulted in the following stack trace:

Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for core.entity_view_display.commerce_product.default.default with the following errors: core.entity_view_display.commerce_product.default.default:third_party_settings.layout_builder.sections.0.components.25d9bddb-abbb-451a-8ab8-1aaa70ee866f.configuration.formatter.settings.redirect missing schema in Drupal\Core\Config\Development\ConfigSchemaChecker->onConfigSave() (line 98 of core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php).

Symfony\Component\EventDispatcher\EventDispatcher::Symfony\Component\EventDispatcher\{closure}() (Line: 206)
Symfony\Component\EventDispatcher\EventDispatcher->callListeners() (Line: 56)
Symfony\Component\EventDispatcher\EventDispatcher->dispatch() (Line: 230)
Drupal\Core\Config\Config->save() (Line: 260)
Drupal\Core\Config\Entity\ConfigEntityStorage->doSave() (Line: 486)
Drupal\Core\Entity\EntityStorageBase->save() (Line: 239)
Drupal\Core\Config\Entity\ConfigEntityStorage->save() (Line: 354)
Drupal\Core\Entity\EntityBase->save() (Line: 617)
Drupal\Core\Config\Entity\ConfigEntityBase->save() (Line: 170)
Drupal\layout_builder\Entity\LayoutBuilderEntityViewDisplay->save() (Line: 311)
Drupal\layout_builder\Plugin\SectionStorage\DefaultsSectionStorage->save() (Line: 159)
Drupal\layout_builder\Form\DefaultsEntityForm->save()
call_user_func_array() (Line: 105)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers() (Line: 43)
Drupal\Core\Form\FormSubmitter->doSubmitForm() (Line: 589)
Drupal\Core\Form\FormBuilder->processForm() (Line: 321)
Drupal\Core\Form\FormBuilder->buildForm() (Line: 73)
Drupal\Core\Controller\FormController->getContentResult() (Line: 39)
Drupal\layout_builder\Controller\LayoutBuilderHtmlEntityFormController->getContentResult()
call_user_func_array() (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 593)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 121)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 183)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 37)
Drupal\Core\Test\StackMiddleware\TestWaitTerminateMiddleware->handle() (Line: 53)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle() (Line: 116)
Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 90)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle() (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 709)
Drupal\Core\DrupalKernel->handle() (Line: 19)
require('[...]/index.php') (Line: 48)

To the future me: I got hold of that trace by the following lines:

    $content = $this->getSession()->getPage()->getContent();
    file_put_contents('/tmp/commerce-test-fail.html', $content);

Thanks @alexpot for the hint.

🇨🇭Switzerland znerol

I've been trying to reproduce the test fails locally (#devdays). I am able to repro some fails sometimes, bot not consistently. Even worse: As soon as I attempt to set breakpoints at the affected locations, tests will pass.

It very much looks like ProductLayoutBuilderIntegrationTest is subject to race conditions - however, I'm not sure whether this issue makes them surface more often.

🇨🇭Switzerland znerol

MR 409 is ready for review.

🇨🇭Switzerland znerol

That's for 11.3, not 11.2

Right, then there is no need to delay the deprecation, I guess.

🇨🇭Switzerland znerol

In both SessionTestController and LegacySessionController, the method documentation often declared a completely wrong return type:

  * @return string
  *   A notification message.

Since this MR touches a couple of those methods, I fixed them all.

I agree that deprecating direct access to the $_SESSION is not disruptive. However, the next step will be to remove support for custom session keys completely. I.e., SessionManager would stop considering custom keys in $_SESSION when deciding whether to start/save a session automatically.

If contrib and custom code continue to access custom keys, this might or might not work depending on other circumstances (i.e., whether or not some unrelated part of the system is using the session as well). But problems will be hard to diagnose because custom keys are silently ignored when the deprecation and support for custom keys are dropped.

That was the reason I think a longer deprecation period could be worthwhile in this case.

🇨🇭Switzerland znerol

Added a legacy test and a CR.

🇨🇭Switzerland znerol

Needs tests for the new deprecation. Also needs a CR for sure.

🇨🇭Switzerland znerol

It is an issue and it is reproducible reliably with the steps in the IS. But probably not easy for test automation (because its a race condition).

🇨🇭Switzerland znerol

If you host a single site on a single deployment: Specify options.uri in drush.yml and place that file in one of the following locations sites/all/drush, WEBROOT/drush, or PROJECTROOT/drush. This will be used as the fallback if nothing else is specified (see drush configuration docs - directories and discovery.

Look into site aliases if you want to do something more complicated. E.g., one site serving multiple domains (example.com, example.org, example.net) or staged deployment (dev, test, staging, production).

Do not attempt to use environment variables unless you are operating in a containerized environment.

🇨🇭Switzerland znerol

Also please study drush configuration docs, especially the Global options section. If the options.uri is configured, any drush invocation will use that as the default value for --uri. This is useful for sites not running on any of the specialized hosting packages (or ddev).

🇨🇭Switzerland znerol

I'd like to avoid the pitfall of wishful/magic thinking here. If drush is supposed to use this feature, then there should be a PoC for drush which demonstrates how this is supposed to be integrated.

Also if all of the projects mentioned in the issue summary and the comments which are supposed to profit from that feature need to be modified, how do we ensure that this is implemented in a consistent way across the board?

🇨🇭Switzerland znerol

I'm confused. Is this setting supposed to have any influence on url generation? Like, e.g., on absolute url in emails sent during a cron run? If yes, how would that work with the current implementation? If no, why add it in the first place?

🇨🇭Switzerland znerol

how to entirely get rid of preprocess via SDCs yet

I'm more likely to implement a twig extension instead of a preprocess function nowadays. Maybe it would be enough to just improve the DX around twig extensions. E.g., attributes:

#[TwigFilter('without')]
public function withoutFilter($element, ...$args) {
// [...]
}

#[TwigFunction('url', options: ['is_safe_callback' => Foo::isUrlGenerationSafe])]
public function getUrl($name, $parameters = [], $options = []) {
// [...]
}

🇨🇭Switzerland znerol

Pushed a test which passes with the fix and fails without.

🇨🇭Switzerland znerol

The assertion triggers inside form validation code. If the site crashes while validating user input, then the validation process is maybe not implemented properly.

How about converting this assertion into an if-condition?

🇨🇭Switzerland znerol

This might be a race condition. I was able to reproduce the trace by dragging the style button to the toolbar, then immediately clicking on save configuration. The moment when I hit the button, the following error appeared in the browser console:

Object { message: "\nAn AJAX HTTP request terminated abnormally.\nDebugging information follows.\nPath: /admin/config/content/formats/manage/basic_html?destination=/admin/config/content/formats%3Fcheck_logged_in%3D1&ajax_form=1\nStatusText: error\nReadyState: 0", name: "AjaxError", stack: "@http://localhost:8888/core/misc/ajax.js?v=11.1.4:196:32\n@http://localhost:8888/core/misc/ajax.js?v=11.1.4:1926:3\n" }

My hunch is that the XHR request following the drag&drop action has been terminated before it had a chance to update the form on the server.

🇨🇭Switzerland znerol

Profile definition in pyroscope:

  // The ids recorded here correspond to a Profile.location.id.
  // The leaf is at location_id[0].
  repeated uint64 location_id = 1;

This mentions leaf. If the other side is the root, then the stack orientation seems to be the same as with excimer.

Also the code which converts the stack trace from otel profile to google profile in pyroscope is not changing the orientation:

	for i := os.LocationsStartIndex; i < os.LocationsStartIndex+os.LocationsLength; i++ {
		gs.LocationId = append(gs.LocationId, p.convertLocationBack(p.src.LocationTable[p.src.LocationIndices[i]]))
	}
🇨🇭Switzerland znerol

The pprof README:

Each sample lists the id of each location where the sample was collected, in bottom-up order.

🇨🇭Switzerland znerol

The order of the stack trace is clearly defined in excimer:

	/**
	 * Get an array of associative arrays describing the stack trace at the time
	 * of the event. The first element in the array is the function which was
	 * executing, the second function is the caller (parent) of that function,
	 * and so on. [...]
         */

Bot not so much in open-telemetry:

  // locations_start_index along with locations_length refers to to a slice of locations in Profile.location_indices.
  int32 locations_start_index = 1;
  // locations_length along with locations_start_index refers to a slice of locations in Profile.location_indices.
  // Supersedes location_index.
  int32 locations_length = 2;
🇨🇭Switzerland znerol

Possibly related warning:

Warning: Undefined array key 33 in Drupal\Component\OTSpanProfile\ExcimerAdapter\LocationIndices->set() (line 49 of vendor/drupal/otspanprofile/ExcimerAdapter/LocationIndices.php)

#0 web/core/includes/bootstrap.inc(108): _drupal_error_handler_real()
#1 vendor/drupal/otspanprofile/ExcimerAdapter/LocationIndices.php(49): _drupal_error_handler()
#2 vendor/drupal/otspanprofile/ExcimerAdapter/Samples.php(52): Drupal\Component\OTSpanProfile\ExcimerAdapter\LocationIndices->set()
#3 vendor/drupal/otspanprofile/ExcimerAdapter.php(105): Drupal\Component\OTSpanProfile\ExcimerAdapter\Samples->add()
#4 vendor/drupal/otspanprofile/SpanProfiler.php(104): Drupal\Component\OTSpanProfile\ExcimerAdapter->getProfile()
#5 vendor/drupal/otspanprofile/ProfileSpanProcessor.php(58): Drupal\Component\OTSpanProfile\SpanProfiler->send()
#6 vendor/drupal/otspanprofile/ProfileSpanProcessor.php(67): Drupal\Component\OTSpanProfile\ProfileSpanProcessor->forceFlush()
#7 vendor/open-telemetry/sdk/Trace/SpanProcessor/MultiSpanProcessor.php(62): Drupal\Component\OTSpanProfile\ProfileSpanProcessor->shutdown()
#8 vendor/open-telemetry/sdk/Trace/TracerSharedState.php(71): OpenTelemetry\SDK\Trace\SpanProcessor\MultiSpanProcessor->shutdown()
#9 vendor/open-telemetry/sdk/Trace/TracerProvider.php(98): OpenTelemetry\SDK\Trace\TracerSharedState->shutdown()
#10 vendor/open-telemetry/sdk/Common/Util/functions.php(36): OpenTelemetry\SDK\Trace\TracerProvider->shutdown()
#11 [internal function]: OpenTelemetry\SDK\Trace\TracerProvider->OpenTelemetry\SDK\Common\Util\{closure}()
#12 vendor/open-telemetry/sdk/Common/Util/functions.php(53): Closure->call()
#13 vendor/open-telemetry/sdk/Common/Util/ShutdownHandler.php(76): OpenTelemetry\SDK\Common\Util\{closure}()
#14 [internal function]: OpenTelemetry\SDK\Common\Util\ShutdownHandler::OpenTelemetry\SDK\Common\Util\{closure}()
#15 {main}

This indicates that LocationIndices algorithm might fail in some scenarios.

🇨🇭Switzerland znerol

I did observe that problem as well. The type hints string|int and array<string|int>|null are the correct choice.

🇨🇭Switzerland znerol

My hunch is this is due to a mismatch between LocationTable and LocationIndices.

https://github.com/grafana/pyroscope/blob/466aaa8194e06aab452b00f56efd5d...

		gs.LocationId = append(gs.LocationId, p.convertLocationBack(p.src.LocationTable[p.src.LocationIndices[i]]))
🇨🇭Switzerland znerol

Nice! Good to see legacy code being removed. I'm impressed by the diff stat: +9 −1291.

🇨🇭Switzerland znerol

I was able to reproduce the problem. It has nothing to do with the PHP version. A fix will be available shortly.

🇨🇭Switzerland znerol

Hi, thanks for the report. Could you please check whether this problem is specific to a certain PHP version? I.e., if you are using PHP 8.4, could you check whether it persists if you switch to PHP 8.3? Thanks.

🇨🇭Switzerland znerol

Use StringTranslationTrait in MailerHooks.

🇨🇭Switzerland znerol

Right. In that case I would revise my suggestion like this:

Creation of a cache table must be an isolated operation. It must run in its own transaction and must not fire any events.

As for the instrumentation, this shouldn't have any influence on application code. I have been evaluating contrib-auto-pdo and this is recording spans for database queries / statements from the very beginning of a request / response cycle if configured properly. No need for events or other application level code in that case.

🇨🇭Switzerland znerol

I am looking from the performance / page cache angle at this issue. Any service which is instantiated before the page cache middleware has a chance to return early is obviously a concern from that PoV.

Unless I'm misunderstanding everything, the problem is this: In order to get the container, it is necessary to perform a cache lookup. If the container cache table is missing, then it has to be created and if that creation operation is supposed to fire events, then obviously the event dispatcher needs to be present.

That problem is unique to the interaction between cache and storage. I cannot see any other thing in the early bootstrap phase where that kind of circular dependency potentially exists.

In other words: There is no non-cache table/collection that needs to be created on-demand before there is a full container.

Arguing from the performance angle again, I'd prefer if the fix is tightly scoped to the problem at hand. A generic pattern IMHO isn't really beneficial. In contrary, a generic solution increases complexity in early bootstrap and potentially causes performance regressions if more "fancy" stuff lands in a position before the page cache.

My suggestion would be to split the table creation logic. Table creation of cache tables must not fire events. Everything else in the storage layer can do.

🇨🇭Switzerland znerol

The log record cannot know the final http status. You need tracing for that.

If you are recording traces and the status code is missing from the spans, then I suggest to open a new feature request.

🇨🇭Switzerland znerol

Yes, sure. I am suggesting incremental improvements.

🇨🇭Switzerland znerol

This is actually one of the things OOP hooks could help with. Let's take a look at Drupal\system\Entity\Menu. All of the overridden methods (preDelete(), save(), delete()) could be extracted to a hook class.

So, my suggestion would be to first try to reduce close coupling of entities with services by leveraging OOP hooks or the event system (whatever works best for the given entity type).

Production build 0.71.5 2024