Account created on 27 March 2012, almost 13 years ago
#

Merge Requests

More

Recent comments

🇷🇺Russia zniki.ru

Thanks for review, PR updated and ready for the review.

🇷🇺Russia zniki.ru

@drdam please provide change record to this issue.
And test doesn't seem to be updated.
Set back to NW.

🇷🇺Russia zniki.ru

Thanks for update MR. New wording looks good, and thanks for provide information about caching.
I check MR again and have one more question.

I love this feature, and I am going to use it as soon it will be deployed. Thanks for your effort.

🇷🇺Russia zniki.ru

Thanks for MR.
Do you think we can add interface for these new methods?

🇷🇺Russia zniki.ru

MR looks good. But should we add test coverage new method at Drupal\Tests\link\Unit\LinkFormatterTest?

🇷🇺Russia zniki.ru

Thanks for MR, this feature looks really nice, but as I understand expected change in the UI already change at #77.
Please update IS.
I left my feedback at MR, please check.

🇷🇺Russia zniki.ru

I was wonder about theme used, but you already mentioned that there is no theme.
I don't clearly understand what does it mean?
You already mentioned limit - "block placing". Are there any other limits?

🇷🇺Russia zniki.ru

Looks like a great idea, thanks.
I left my feedback on MR.

🇷🇺Russia zniki.ru

Add internal links.
Improved error message. Provide violation message.

🇷🇺Russia zniki.ru

I don't understand why review bot fails on phpstan. MR check looks good.
Add tag to not ignore bot.

@chi this looks like a good suggestion. I believe there are more places where this can be done.

🇷🇺Russia zniki.ru

I mad MR 10993. MR 11001 looks like good fix. @godotislate thanks.
+1 for RTBC

Yes, it's very concerning if functional javascript tests have false negatives on MR builds.

I think this false positive. Tests passes but it should fail.

@nicxvan thanks for double check original MR, so do I and also found nothing.

🇷🇺Russia zniki.ru

Looks like some random fails. Please double check.

🇷🇺Russia zniki.ru

Thank you @berdir, @quietone. For the links and the explanations.

🇷🇺Russia zniki.ru

Thanks @nicxvan for review, I update MR.

🇷🇺Russia zniki.ru

Create MR.
Not sure that implement points 2, 3 with changes at

  • Drupal\hook_collector_on_behalf\Hook\OnBehalfOfOtherModuleHook
  • Drupal\hook_collector_skip_procedural\Hook\SkipProceduralHooks

is perfect way.
Maybe we need to check it at UnitTest "Drupal\Tests\Core\Hook\HookCollectorPassTest", but I don't know how this can be achieved.

🇷🇺Russia zniki.ru

Looking forward to merge this. I really need this for several projects.

🇷🇺Russia zniki.ru

Thanks @nicxvan, I remove special handling for "hook_hook_info()".

🇷🇺Russia zniki.ru

@klausi thanks for your review, I update PR and it's ready for review.

@nicxvan, I add special handling for hook_hook_info(). But still not sure about it.

🇷🇺Russia zniki.ru

Thanks, it's very food question.
Because when combine them together and now use base class, it will be test with more then 500 lines of code.
It's easy to read and understand for me.
But do you have any suggestion?

🇷🇺Russia zniki.ru

Create draft MR, need to decide what other internal links to test.

🇷🇺Russia zniki.ru

Thanks for all you effort, but from my side this doesn't look like bug.
It looks like default behavior.
My suggestion if you want this behavior create custom formatter, or contrib module that implements this.

🇷🇺Russia zniki.ru

I revert changes that is not a part of actual scope. You can see commit dc3a17f7.
These can be implemented in follow up to improve test speed for Drupal\Tests\link\Functional\LinkFieldTest :

  • Improve ::assertValidEntries(), ::assertInValidEntries()
    Check multiple values at single request.
    Check exact value, instead of whole page.
  • use EntityTest::create() everywhere in test.
  • do not add field to ViewDisplay, FormDisplay if it's not used.
  • use static field name instead of random field, to make easy to debug test

MR is ready for review.

🇷🇺Russia zniki.ru

Found way to improve assertInvalidEntries speed.
But I don't understand why when LinkWidget::validateUriElement throws error "Manually entered paths should start with one of the following * characters: / ? #" all other errors are hidden. Other errors provided not by element validation, but with constraint.

I would say this is bad UX, because for example you submit several invalid links, and at first submission you see only one error. You fix it, submit again, and then you see that there are other errors. I searched through issues, and found nothing related to this.

MR is huge, I decide to move test speed improvements and refactor to separate issue.

🇷🇺Russia zniki.ru

nikolay shapovalov changed the visibility of the branch 3418184-improve-test-speed to hidden.

🇷🇺Russia zniki.ru

I spend some time trying to generate error without Migrate UI, but have no luck. I agree with @benjifisher to keep test as is.

🇷🇺Russia zniki.ru

testNoLinkUri() now can be completely removed. Because <nolink><none><button> field values added to new kernel tests for default link formatter and separate link formatter. And add new valid_entry <button> field value for method doTestURLValidation().

I have few suggestions:

  • improve test speed for doTestURLValidation(). Some changes already made at branch 3418184-improve-test-speed.
  • change method name to doTestUrlValidation().

Should these suggestions implemented in this issue?

Set back to NR to get some feedback. IS updated.

🇷🇺Russia zniki.ru

Set to NW because want to convert to kernel testNoLinkUri()

Update MR. Added <button> <none> <nolink> to already converted tests. And replace array filter with simple string values. Also make check for each field delta separate, and use assertEquals.

🇷🇺Russia zniki.ru

Made some research on method doTestURLValidation() and found few ways to improve tests speed.
I think some of tests can be moved to constraint validation test.
Push changes to new branch.

🇷🇺Russia zniki.ru

Not sure I follow what in testNoLinkUri is the concern about being in kernel?

Do you think it should be converted to kernel, or keep this functional?

I doubled checked testNoLinkUri(), we are testing widget at doTestURLValidation() for links

  • <nolink>
  • <none>

We can add <button> to doTestURLValidation() and then completely remove testNoLinkUri(). Also add these cases to new Kernel tests.

If we are not going to make any changes for testNoLinkUri(), we should convert from 3 requests to 1 request.

🇷🇺Russia zniki.ru

I moved functions

  • deprecation_test_deprecated_hook
  • deprecation_test_deprecated_alter_alter

to Drupal\deprecation_test\Hook\DeprecationTestHooks class
And it become very similar to Drupal\deprecation_hook_attribute_test\Hook\DeprecationHookAttributeTestHooks.

Should we remove module deprecation_hook_attribute_test at all?
Or is it better to remove class Drupal\deprecation_test\Hook\DeprecationTestHooks?

🇷🇺Russia zniki.ru

LayoutBuilderUiTest::testNewExtraField doesn't test content display.
It just test layout_builder admin manage page: admin/structure/types/manage/bundle_with_section_field/display/default/layout. And this test doesn't check entity display at all. That the reason function layout_builder_extra_field_test_node_view can be removed.

🇷🇺Russia zniki.ru

I decide to remove layout_builder_extra_field_test_node_view(), IS updated.

🇷🇺Russia zniki.ru

Looks like layout_builder_extra_field_test_node_view can be simply removed. Because we have LayoutBuilderExtraFieldTestHooks::hook_entity_extra_field_info

🇷🇺Russia zniki.ru

@nicxvan. Thanks for review. Meta issue is great, thanks for all your hard work.

🇷🇺Russia zniki.ru

Next time I will use search better.
I tried to do pretty similar thing but with different approach at 📌 Replace usage of dblog module at tests with alternative Active .
And I am very happy about approach used at this issue. Having this at Kernel test by default will improve DX a lot, because now there are tons of different ways doing this.

I would suggest not to replace tokens at message, but keep original message and provide tokens value. Because at FormattableMarkup::placeholderFormat there are much changes, not just strtr().
And we are not checking message display but provided values.
And it much easy to search message source.

You can check example at MR 10863, but here is some code for short example.

     $expected_message = [
      'message' => "View display '@id': Comment field formatter '@name' was disabled because it is using the comment view display '@display' (@mode) that was just disabled.",
      'context' => [
        '@id' => $host_display_id,
        '@name' => $field_name,
        '@display' => EntityViewMode::load("comment.$mode")->label(),
        '@mode' => $mode,
      ],
    ];
    $this->assertTrue($logger->hasRecord($expected_message, RfcLogLevel::WARNING));
🇷🇺Russia zniki.ru

Drupal CI is no longer used, need to update article.

🇷🇺Russia zniki.ru

nikolay shapovalov changed the visibility of the branch 3496184-convert-smart-default-settings to hidden.

🇷🇺Russia zniki.ru

nikolay shapovalov changed the visibility of the branch 3496184-logger-test-class to hidden.

🇷🇺Russia zniki.ru

I created a Proxy for \ColinODell\PsrTestLogger\TestLogger to filter context of log records.
And result looks good. All \ColinODell\PsrTestLogger\TestLogger methods can be used.
Probably this is better than creating custom TestLogger from scratch.

🇷🇺Russia zniki.ru

I create helper class TestLogger. Attaching it directly in the test method looks very easy, and hasn't a lot of boilerplate.
I was thinking to use colinodell/psr-testlogger, but there are a lot of data in the log->context. In order to filter it, we can use proxy and facade approach, but I want to keep it simple. Creating custom class looks like good alternative.
Also I added rendered message to log, and TestLogger can search rendered message, this is not used in MR, but this should be useful for someone who is searching simple rendered message.

Set to NR to get some feedback.

🇷🇺Russia zniki.ru

I combined methods LinkFieldTest::testLinkFormatterQueryParametersDuplication() and LinkFieldTest::doTestLinkFormatter(), and move it to class Drupal\Tests\link\Kernel\LinkFormatterDisplayTest.
method LinkFieldTest::doTestLinkSeparateFormatter() to class LinkSeparateFormatterDisplayTest.
method LinkFieldTest::doTestLinkTypeOnLinkWidget() to class LinkFieldWidgetTest.

And created base class for LinkFormatterDisplayTest, LinkSeparateFormatterDisplayTest.
Looking for feedback.

🇷🇺Russia zniki.ru

@nicxvan thanks for review and creating CR. CR looks good.
Please take care of deprecation, if we will decide to use deprecation.

I think CR is enough, but let's wait for someone else feedback as well.

🇷🇺Russia zniki.ru

+1 for RTBC. Thanks @dillix

🇷🇺Russia zniki.ru

Converting doTestLinkTypeOnLinkWidget was not just copy and past, had to create ViewDsiplayForm and then attach field. Not sure if the tests doesn't change itself, maybe I miss something.

doTestLinkFormatter and doTestLinkSeparateFormatter looks very similar to testLinkFormatterQueryParametersDuplication before refactor. I will refactor them as well.

🇷🇺Russia zniki.ru

@benjifisher thanks a lot for great review and your changes to IS. I applied suggested change to second assert and update fork to latest 11.x.

GitLab CI has a test-only job. (It is not run automatically. You have to trigger it when you want it.) We do not need a test-only branch, so I am hiding the one you added.

Thanks for sharing information about test-only job, didn't know.

🇷🇺Russia zniki.ru

@nicxvan thanks for feedback.
But as I can see colinodell/psr-testlogger is already a part of Core dependency.

I will ask for advice anyway, thanks. Just want to make some research before, to have better understanding of the topic.

🇷🇺Russia zniki.ru

Thanks for update @dillix.
I found some more issues, please fix this.

🇷🇺Russia zniki.ru

I decide that it would be great to have TestLogger to use at tests as dblog alternative.
And found that we already using \ColinODell\PsrTestLogger\TestLogger at some tests.
Another great example is \Drupal\Tests\feeds\Kernel\TestLogger and Drupal\Tests\feeds\Kernel\FeedsKernelTestBase.

🇷🇺Russia zniki.ru

Made change to MR, to use Drupal service instead of container. Because of 🌱 Use \Drupal consistently in tests Needs work .

🇷🇺Russia zniki.ru

I double check LinkFieldTest.php, there methods also can be moved to Kernel:

doTestLinkFormatter - http request to create entity.
doTestLinkSeparateFormatter - http request to create nodes, but we are testing formatter. I beleive we can create nodes with code.
doTestLinkTypeOnLinkWidget - no http request, simple convert.

testNoLinkUri - http used to creates nodes. It looks likes this is test for both widget and formatter. Not sure if it's good idea to convert to kernel. What do you think?

And in order to move to Kernel we need to move renderTestEntity to trait.

I double check existing MR and changes I made is not only about moving testLinkFormatterQueryParametersDuplication() to Kernal, but also about refactoring, this is not just copy/paste. I update issue summary and title, to correspond suggested changes. And we can focus on other methods at follow-up issue.

Production build 0.71.5 2024