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.
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.
- flaky_tests_data is a repository with the results. A pipeline is rebuilding it daily. The data is also accessible via GitLab Pages (e.g., latest.json).
- flaky_tests_scraper code for the scraper.
- flaky_tests_dashboard a small PoC which lists a summary of failing tests in a given dataset.
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.
Latest data is accessible via https://project.pages.drupalcode.org/flaky_tests_data/drupal/latest.json
@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.
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_...
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.
Tests are passing now.
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.
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.
Added a CR.
MR 409 is ready for review.
That's for 11.3, not 11.2
Right, then there is no need to delay the deprecation, I guess.
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.
Added a legacy test and a CR.
Needs tests for the new deprecation. Also needs a CR for sure.
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).
Closing this as outdated.
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.
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).
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?
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?
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 = []) {
// [...]
}
Nice approach!
Pushed a test which passes with the fix and fails without.
Working on tests.
Pipeline passed.
That work was completed in #2286971: Remove dependency of current_user on request and authentication manager → .
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?
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.
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]]))
}
The pprof README:
Each sample lists the id of each location where the sample was collected, in bottom-up order.
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;
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.
I did observe that problem as well. The type hints string|int and array<string|int>|null are the correct choice.
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]]))
Nice! Good to see legacy code being removed. I'm impressed by the diff stat: +9 −1291.
I was able to reproduce the problem. It has nothing to do with the PHP version. A fix will be available shortly.
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.
Use StringTranslationTrait in MailerHooks.
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.
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.
I guess yes.
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.
Yes, sure. I am suggesting incremental improvements.
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).
Conflicts due to 📌 Use ::class instead of string literals in plugin managers Active resolved.
Thanks! Maybe @peter törnstrand could check whether that works in Seq as well.
Thanks. Took this to a test-drive with loki and grafana. I noticed that if there is a backtrace, then it appears both in the code.stacktrace attribute as well as in the log body. Grafana displays the whole log body of each row in the explore view, and that can easily fill my laptop screen if an exception was thrown deep enough in the stack.
In order to prevent that, I'm setting $message_placeholders['@backtrace_string'] = '' before replacing the placeholders into the message in otlog.
There are some minor code style issues flagged by eslint (patch can be downloaded from the job) and cspell.
Took a look at SysLog and found that they are using strip_tags() on the $message and also on $context['link'].
I was curious on how links are stored in the database, so I took a look (SELECT link FROM watchdog WHERE link<>"";). Result:
<a href="/node/1" hreflang="de">Ansicht</a>
[...]
That is unfortunate:
- href contains a relative path
- After
strip_tags(), only the link text will be visible (Ansicht)
Since this is totally useless, I'd suggest to leave out the link field altogether.
Re #21: Fair.
Maybe we need a way to create two (or more) groups of routes. One group is accessed by browsers, others by special clients. Then apply different sets of rules for request processing, content/language negotiation, authentication and caching to each of them.
However, page cache is only effective if it runs before request routing. So maybe the matching needs to happen on a path prefix.
smustgrave → credited znerol → .
Looking at DbLog.php, it appears that there is a finite list of supported context values which are not placeholders:
- channel
- ip
- link
- referer
- request_uri
- timestamp
- uid
Everything else is lost unless it matches a placeholder in the message template. The remaining context keys could easily be matched manually together with uid, request_uri and ip:
$attributes = [
TraceAttributes::USER_ID => $context['uid'],
TraceAttributes::URL_FULL => $context['request_uri'],
TraceAttributes::CLIENT_ADDRESS => $context['ip'],
TraceAttributes::HTTP_RESPONSE_HEADER . '.referer' => $context['referer'],
'drupal.log.channel' => $context['channel'],
'drupal.log.link' => $context['link'],
];
I think that the only change this issue fixes, then, is setting an
Expiresheader which honorssystem_performance.cache.page.max_age, which would be possibly beneficial for HTTP 1.0 proxies down the line of a response.
No, that wouldn't be desirable. If Drupal did this, then that would lead to potential information leak via HTTP/1.0 caches. This is because HTTP/1.0 caches would store/serve every page, regardless of whether there were cookies on the request or not. HTTP/1.0 caches are unable to differentiate between authenticated requests and anonymous ones simply because they do not know anything about cookies.
Bring the discussion of max_age config parameter honoring to 🐛 [pp-3] Bubbling of elements' max-age to the page's headers and the page cache Postponed
Depends. If you mean that no page should be cached in the page cache longer than system_performance.cache.page.max_age, then no. If you mean that page cache should honor the max-age directive collected from render metadata, then yes, that is the correct issue.
Create another issue to fix the mentioned remaining problem with PageCache inspectiong Expires header instead of Cache-Control, so this one don't get "noisy"?
No need for that, this is covered by 🐛 [pp-3] Bubbling of elements' max-age to the page's headers and the page cache Postponed .
That seems to be still unclear upstream (see this GH issue). Personally, I'm leaning towards a fixed value for the instrumentation name/logger name. I have no particular reason other than a preference to keep things simple.
You mean something like this @berdir?
// Extract context values to attributes not interpolated in message.
$extraAttributes = array_filter(
$placeholders,
fn (string $key) => !str_contains($message, $key),
ARRAY_FILTER_USE_KEY
);
unset($extraAttributes['uid']);
unset($extraAttributes['request_uri']);
unset($extraAttributes['ip']);
foreach ($extraAttributes as $key => $value) {
$record = $record->setAttribute($this->extraAttributeKey($key), $value);
}
The extraAttributeKey() method would need to sanitize the context value key and add a prefix (e.g., org.drupal.channel).
Sorry for the late answer, commenting on the issue summary and the two points raised by @Wim Leers:
1. Drupal 8 intentionally disables HTTP/1.0 proxies
This has been the case since Drupal 7 (and also Pressflow - Drupal 6 optimized for high performance content oriented websites). It has been necessary for any web application which uses cookies to disable HTTP/1.0 caches. There is no way in HTTP/1.0 to maintain different page variations under the same URL. Everything which influences the result (like desired language, session and login state) has to be encoded in the URL. Yes, session identifiers were passed via request parameters back then.
When HTTP/1.1 introduced cookies and cache control, it was specified that the Cache-Control: max-age directive takes precedence over the HTTP/1.0 Expires header.
"Modern" web applications which serve multiple variants of a page under the same URL (depending on the application state maintained in the cookie header) can be made compatible with both HTTP protocol versions by serving a suitable Cache-Control and Expires header. The way to do this is to serve an appropriate Cache-Control header to permit storage in HTTP/1.1 caches (if appropriate) and serve an Expires header which is set into the past in order to disable caching in HTTP/1.0 caches. Remember, HTTP/1.0 caches assume that the only thing which can influence the response is the URL.
This pattern is not specific to Drupal. Everybody does this.
Drupal 8 ships with a HTTP/1.0 reverse proxy that is breaking the spec: Page Cache
Responses that have Expires: Sun, 19 Nov 1978 05:00:00 GMT are meant to not be cached, because it's an expiration date in the past. Yet PageCache caches them permanently:
The page cache module is not a HTTP/1.0 reverse proxy. It is neither a HTTP/1.1 reverse proxy. The caching logic has its roots in Pressflow and is tailored to content oriented websites.
In unmodified HTTP/1.1+ reverse proxies and browser caches, the only way to get rid of cached pages is to limit their validity to a certain time frame (i.e., max-age or s-max-age). The system.performance.cache.page.max_age config value is the way to specify that value for those downstream caches which are otherwise out-of-reach for Drupal.
With the built-in cache the situation is different. It is possible to remove cached pages whenever a change is made on the website. Stock Drupal 7 did nuke the whole page cache whenever any content was changed. Some contrib modules rectified that heavy handed approach and made it possible to define rules which pages were purged depending on which content was changed.
D8+ introduced cache tags and that removed all the guess work. No site builder has to think about which pages to purge from the cache nowadays.
For this reason the page cache module keeps rendered pages in the cache for an unspecified amount of time. This has been the way it worked since Pressflow.
The following remark in the issue summary is indeed pointing to a problem point in the page caching module:
PageCache determines which responses to cache by inspecting the Expires header. (This should be updated to use the Cache-Control header instead, but that's out of scope here.)
The page cache module really shouldn't inspect the Expires header. That logic needs to be deprecated indeed.
Updating issue status to Postponed (maintainer needs more info). I will wait a couple of weeks and then set it to Closed (works as designed) unless there is significant new (and valid) information added to the issue summary.
we can unrevert it :D :D
That wouldn't resolve the bug, though. The hook registry needs to be separate from EventDispatcher.
The implementation from
#1972300: Write a more scalable dispatcher →
could certainly be reused for ModuleHandler.
Reading through the current code and the patch, it looks like the most significant change regarding the message body is that before it was structured and after it is flat (a string).
From that I conclude that Seq requires the message body to be a string. In order to fix the bug without breaking existing deployments.
Also it looks like some backends will extract the backtrace from attributes (mine does), while others don't. So, it might be sensible to just make the body format configurable.
Message body configuration knobs could include:
- Message body as string vs. body as object
- Include stack trace in message vs. exclude it
Also note that the log APIs seem to be still in flux in the OT spec. E.g., v1.39.0 and v1.41.0. So maybe wait with refactoring until the spec trickled down into the OpenTelemetry PHP package.