🇫🇷France @goz

Issue comes from ajax link js which does not take care of language calling itself.

line 107 in ajax-link.js should use Drupal.url()

const url = Drupal.url(`ajax/ajax_link?path=${encodedHref}&selector=${encodedSelector}&method=${method}`);

Module resolve path based on current language, and take user preference first.

goz → created an issue.

goz → created an issue.

You are right, it's fixed in 1.0.0-beta2

Branch has been rebased and tests are green

Tests fail, but nothing relative to current changes.

Anyway, the branch has to be rebased.

Metrics values have been updated due to rebase.

I have updated previous asserts to replace them by the new assertMetrics().

Except for core/profiles/demo_umami/tests/src/FunctionalJavascript/AssetAggregationAcrossPagesTest::testFrontAndRecipesPagesAuthenticated(), where i kept assertLessThan() for ScriptBytes because values changes at each call, and it's not the purpose of this issue to fix that.
$this->assertLessThan(250000, $performance_data->getScriptBytes());

Hi @pdureau, i'm working with njaber, on this issue (and i'm the one who ask to open issue).

So let me give more details.

In the case we have a menu with some first-level elements which give access to sub-menu elements and we do not have permissions to see those sub-items, elements have no children. In this case, there is no reason to have a item to display.

In any cases, if item is a , it should not be displayed as a link.

In this case, route manipulation and checking are done by Drupal core mechanisms, but this give us a UI result with a which should be used to display other links, except there is no more links to display and finally display the as a link.

Here is an example of suggested fix :

          {% if item.below %}
            {% set item_id = 'menu-' ~ loop.index ~ '-' ~ random() %}
            <button class="fr-nav__btn"
                    aria-expanded="false"
                    aria-controls="{{ item_id }}"{% if item.in_active_trail %} aria-current="true"{% endif %}>
              {{ item.title }}
            </button>
              <div class="fr-collapse fr-menu" id="{{ item_id }}">
                {{ _self.menu_links(item.below, attributes, menu_level + 1) }}
              </div>
          {% elseif item.url.routeName != '<nolink>' %}
              <a class="fr-nav__link"

The two first branches have been hidden.

goz → changed the visibility of the branch 3477191-add-performancetesttraitassertmetrics-so to hidden.

goz → changed the visibility of the branch 3477191-add-assertmetrics-expecteddata to hidden.

goz → created an issue.

Last MR to implements the way talked in #20 #21.

An implementation example :

    $expected = [
      'QueryCount' => 124,
      'CacheGetCount' => 227,
      'CacheSetCount' => 190,
      'CacheDeleteCount' => 1,
      'CacheTagIsValidCount' => 37,
      'CacheTagInvalidationCount' => 0,
      'ScriptCount' => 1,
      'ScriptBytes' => 212313,
      'StylesheetCount' => 1,
      'StylesheetBytes' => 29911,
    ];
    $this->assertMetrics($expected, $performance_data);

gitlab-ci has been added, and linters have been fixed by 📌 Fix linters from gitlab-ci results Active

goz → created an issue.

goz → created an issue.

Still work on this, but needs founders

The assertMetrics() method should be responsible for setting ranges when appropriate. In the example above, it would create these assertions

What do you mean by "when appropriate" ?
Even if we define stylesheetsCount has to be in a -50/+50 ranges, how do you deal with tests which need another range ?

Why i like array + assertMetrics() : just one array to define + one assertMetrics to call.
Why i dislike it : Needs to copy/paste from another tests or search about key strings. Against an object that will provide auto-completion in IDEs. And how to deal with range expectations in the array ?

Monitored Room 6 for Gander workshop

goz → created an issue.

This does not reduce the amount of lines to write to make tests, but make metrics tests easier to read.

Here is the changes :

• Add an ExpectedPerformanceData object that inherit from PerformanceData.
• Add the optional range parameter when setting expected datas. By default, Bytes tests are configured with a 50 range.
• If no data is set in ExpectedPerformanceData object, the data will not be asserted.
• To not assert queries, use $expected_performance_data->disableQueries()

If some new metrics are added, we will have to add them to PerformanceData and extend setter in ExpectedPerformanceData to add range parameter + add the range getter and variable

#14 sounds great. I open a new branch to work on it

Is it worth thinking about passing in two $performance_data objects instead of multiple args or an array? We could add extra things like acceptable ranges in their own properties.

@catch i'm not sure to understand what you mean here. Here is what i think you mean :
For each metric in the performance data object definition, we hard code that for this metrics, it allows a range (or not). For example, stylesheets bytes have an acceptable range of 50, so if you make an assert of 1000, it will allow by default values between 950 and 1050.
But for cache query count, we does not want a range, so if range configuration is set to 0, it means the value has to be the same.
Of course, this range configuration could be overridden if test need it.

@pavel.bulat the point to make an assertMetrics() with all parameters was to write performance tests faster. I agrees it's harder to know which parameters is made for without good IDE.
However, building an array with metrics names/values is equivalent to asserting each value, except you have to give the exact name which is not auto-completed, even by IDE.

If we want to have clean message and not only the breaking line, we can prepare the test with method in trait like expectMetricsForQueryCount(10), expectMetricsForCacheGetCount(0) (...) and then call an assertMetrics($perforance_data) which will launch asserts based on "range properties" defined. That allows to add another metrics later.

In any cases, i'm not see any solutions to help developer writing tests AND keep readable tests AND not doing copy/paste.

@benjifisher

Is it always the same metrics that need ranges? If so, then I think it would be simpler to pass, say stylesheet_bytes: 30550 and have assertMetrics() automatically use a range:

No it's not, in some cases, like in 📌 Add assertions to OpenTelemetryFrontPagePerformanceTest::testFrontPageColdCache() Active , we also have variations for cache counts.

If different tests need different ranges, then we need something more flexible, like your proposal. But if we end up always using +50/-50, and always for the same metrics (byte counts, cache tag counts) then we should keep it simple.

Agreed, except the range is not always the same, depending of the complexity of the loaded page i guess. That's why i choose do define the range manually.

If we are going to use an array for a range, then I think we should use a base value and a delta, like [30550, 50], and let assertMetrics() convert that into the range [30500, 30600]. This will make it easier to write the tests (Let the computer do the arithmetic) and it will be clear what actual values we found while we were writing the test.

You will still have to to arithmetic to find out range and delta. If in the first test, we have 120000, the second 130000, the third 180376 we still have to figure out which first number to put, and which delta. Anyway, in much cases the first count we get + 100 or 50 delta should be enough.

Anyway, in this case, assertEqualsWithDelta() should be more relevant than assertCountBetween().

Again, I think we should agree on the order of parameters. (See Comment #2.)

That depends of the relevance of those metrics, and i don't know which is more relevant than another. In any cases, if we always need them all, order is not a big deal. If i had to choose, may be the query count is the one i'll put at bottom.

I like the idea, but we should also deal with range asserts for cases where we expect some variations in count.
I suggest to allow null, int and array, so we can pass minimum/maximum range and then call assertCountBetween().

protected function assertMetrics(
    PerformanceData $performance_data,
    int|array|null $stylesheet_count = NULL,
    int|array|null $script_count = NULL,
    int|array|null $stylesheet_bytes = NULL,
    int|array|null $script_bytes = NULL,
    int|array|null $query_count = NULL,
    int|array|null $cache_get_count = NULL,
    int|array|null $cache_set_count = NULL,
    int|array|null $cache_delete_count = NULL,
    int|array|null $cache_tag_checksum_count = NULL,
    int|array|null $cache_tag_is_valid_count = NULL,
    int|array|null $cache_tag_invalidation_count = NULL,
  ): void {

Here is the example call for NodeAddTestPerformance 📌 Add performance test coverage for the node/add page Active

$this->assertMetrics($performance_data, 1, 1, [30500, 30500], [212000, 213000], 124, 227, 190, 1, [30, 127], [30, 39], 0);

To help finding expected values when we write tests, i also add a getMetrics() method which return an array with all metrics.

I also add new rules on PerformanceTestTrait::logQuery() to fix queries matching issues.

Monitored Room 6 "Lazy loading content for better performance, SEO and UX"

Monitored Room 4 "Introducing the new access policy"

Fail is not relevant. Let's relaunch it

There was 1 failure:
1)
Drupal\Tests\image\Functional\ImageStylesPathAndUrlTest::testImageStylePrivateWithConversion
Behat\Mink\Exception\ExpectationException: Current response status code is
403, but 200 expected.

Monitored room 4 for Why a big deal Drupal sync is

Monitored room 4 for SearchAPI meets typesense

Monitored room 4 about UI patterns 2

Monitored room 2

Monitored Room 6 for Gander workshop

Monitored Room 6 for Gander workshop

Failures were false. MR passes tests after a relaunch

Queries are not deterministic anyway, so i remove them.

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

We could add ckeditor5 and others but which one ?
And does it make sense ?
The purpose it to test performance of node/add page, not depending of field types right ?

I add cold and hot tests, but still have to deal with warm tests and maybe ckeditor5.

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

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

After talking with Janez (slashrsm) after drupalcon workshop, there is no point to use "assertLessThan", we should test the exact byte.

I arbitrary put limit to 41880.

There was 1 failure:

1) Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryNodePagePerformanceTest::testNodePage
Failed asserting that 41878 is less than 40400.

/var/www/html/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryNodePagePerformanceTest.php:74
/var/www/html/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryNodePagePerformanceTest.php:31

I'm working on this at DrupalCon Barcelona

I agree. Entity should not duplicate base fields but extends and use ContentEntityBase.

However, while 🐛 Vote entity unnecessarily re-implements features provided by base class Needs work is not fixed, if someone needs uuid support, the current patch and issue add it. We cannot do better in this issue without big changes in the module

Thanks @Webbeh

We are now in D10 (even D11), and this duplicate 📌 Automated Drupal 10 compatibility fixes RTBC

By default, block entity has a status property. So if you don't have a status field displayed, I guess your block is created with status published.
Scheduler field assumes you know what you do when you set status editing an entity (and that you can change the status). It assumes that if you plan to publish an entity in the future, you already create it unpublished.

May be it could be a new feature to enforce status to published/unpublished entity (or every other scheduled actions) when it's saved depending of scheduler field configuration

In the meantime, perhaps you can find a module that give you access to the status field of a block in layout builder?

Do you create your block as unpublished? So with status = false?

I have the same issue with Drupal 10.3.3 and votingapi 3.0.0-beta5 :

Parameter "vote_result" for route "jsonapi.vote_result--vote_result.individual" must match "[^/]++" ("" given) to generate a corresponding URL.

The following issue solves this ✨ Add support for UUID field to votingapi_result table Needs review

I need this to make vote_result entities available with jsonapi.

Here is a patch update for last 8.3.x and Drupal 10.

UUID is a requirement to make entities available on JSONAPI. An issue on core deal with a better way to detect an entity has missing UUID #3090253: Detect when UUID is missing and provide better exception/error message when constructing a JSON:API payload → but UUID will still be required.

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

Maintainers have been contacted from d.org

goz → created an issue.

Hi, i confirm this issue is RTBC and module works fine with D10

Thank you both of you. It was fast !

I understand we keep it opened with postponed status to find a way to fix it later, except the subject of the topic is not accurate anymore.
Should we rename the issue again to reflect the postponed subject or add a child issue with a postponed status ?

goz → created an issue.

Thinking about it, if we want to use form error and avoid issues reported by my previous post, maybe we could clear values after setting form error, so malformed values (array) cannot report error or be a risk anymore.

I suggested (and push for it) to use 500 error instead of form error for the following reasons :

- in my tests, if I set a form error, other validators still process, and also fail with 500 (with generic answer, hard to diagnostic)
- this issue can occur mainly for two reasons : an attack, or bad code. In second case, developer should deal with the 500 error in its tests.
- on cases reported by this issue, this issue appear because of bad intentions form an attacker. We should stop it as soon as possible (so 500)

GoZ → created an issue.

GoZ → created an issue.

I think it would be relevant to add a warning on top of this main documentation and https://www.drupal.org/drupalorg/docs/drupal-ci/customizing-drupalci-tes... → to redirect people who whant to make tests on their projects to use GitlabCi.

What do you think about this ?

Nice catch. i also add feature to not provide a scheduler type option if it cannot be available for the current entity.

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

Any update on this ?

What do you think about this patch ?
What about my proposal dispatching an event to alter ajaxResponse ?

I check on fresh installation and field exists in profile

Thanks all

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

Thanks sarwan, but #2 is wrong.
I fix it following https://www.drupal.org/node/3158256 → tips

I have the same issue in my logs with query like :
curl http://localhost/user/register?element_parents=account/mail/%23value&ajax_form=1&_wrapper_format=drupal_ajax" -X POST -d "form_id=user_register_form&_drupal_ajax=1&mail[#post_render][]=exec&mail[#type]=markup&mail%5B%23markup%5D=TRY_TO_INJECT_SOMETHING"

I wonder if it make any sense that a field with #maxlength have another #value than a string.

My first attempt would be the same a #13, except we should not receive an array here. May be we should :

1. At minimum set form error if @maxlength is setted, but #value is not string. In this case, we cannot prevent some other code will break because we don't expect value is other thing than string
2. I think the best is to raise a 500 exception since it should not happen