🇧🇪Belgium @borisson_

@peter törnstrand https://www.drupal.org/node/3442349 →

Directly moving this to rtbc, since it's not only widens visibility, so it cannot be considered an API break, nor can it break other implementations.

I agree, making this private breaks the possibility for us to extend this class, if it is not intended to be extended, it should probably be final so it's not even possible.

I agree that the documentation is not up to date as-is and we should update it to be clearer, was just looking into how to set this up and we could only find the link to the drupal 7 documentation, with a big banner saying that drupal 7 is no longer maintained.

Can we make sure the drupal 8 documentation is on drupal.org, or place a prominent link on the module homepage to the readme.txt?

White this is really difficult to figure out how to trigger this without using any contrib. I seem to have the same issue when running database updates with drush when using a subtheme of the bootstrap theme. This breaks hard and gives an error I don't know how to pass.

The patch by @jungle works great to work around this issue and seems like a simple hardening of the code.

I updated steps to reproduce, but I'm not sure if they are enough or simpler to do. This is for a project where I didn't do the setup myself.

@smustgrave, you might know how we should proceed here? Using a contrib theme is the easiest way to reproduce the issue.

Thanks @mlncn!

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

Should this be considered a BC break, or can we commit this without a problem?

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

Looks like I am allowed to merge this, but not to create a new release. I sent a ping to @karthikeyan-manivasagam

Thanks Dieter!

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

Thanks for fixing this @robindh!

See 📌 Automated Drupal 11 compatibility fixes for webform_rrn_nrn Needs review

See 📌 Automated Drupal 11 compatibility fixes for webform_rrn_nrn Needs review

See 📌 Automated Drupal 11 compatibility fixes for webform_rrn_nrn Needs review

Fixed, committed and pushed.

What is holding back the merge of this fix?

Several things, but the first being that this issue is still at needs work.
@quietone did a really big review a year ago, that review needs to be addressed (the questions answered and the proposed changes implemented).
After that is done, this issue can go back to needs review and it can hopefully reach RTBC.

So the first step is addressing the review.

However it would rely on the same end of request logic so doesn't help with the above search_api/automated_cron/views issue. How come a view is getting rendered when search indexing a node?

Search API can be configured in multiple ways, for example it can be configured to only index certain fields. However the recommended way, which is also implemented here, is that the rendered html is used for indexing content. This way all kinds of alters and preprocesses are included in the final content, as well as all kinds page building techniques can be used for building the final representation of that content.

borisson_ → created an issue. See original summary → .

borisson_ → created an issue. See original summary → .

esmoves → credited borisson_ → .

Looks like the maintainer committed your issue, I think that means this issue can be closed? At least it means they are active.

Tests are happy about the latest pull request. I think this can go in.

The changes done here by @znerol are the right way to go, these look to be is the most minimal changes needed to get d11 compatibility.

I would suggest to merge those and tag a new release, the followup issues created can be done in the next release imho

This is just a .info.yml update, so I think it can be committed and a new release can be launched as well.

Both of these merge requests should be committed, after that a new release should be helpful for people trying to upgrade.

This looks great, I think we should commit this, and release a stable version of the module

The latest release is d10 compatible, I think this issue should be closed.

The merge request has merge conflicts that need to resolved. I also think there needs to be better test coverage, it is currently only tested through core/modules/field/tests/src/Functional/EntityReference/EntityReferenceXSSTest.php.

Setting to needs review because there is a patch

Commented on the issue in gitlab. I think the solution here is not correct or indicates an issue in core itself.

Since this is only a .info.yml update, this can be applied without much stress by the maintainers. I think it would be helfpul to also get a tagged release after this

Looks great, I hope we can commit the merge request from @jeroent. Especially since this also fixes the tests I think it should be committed.

To make it easier for people to upgrade, I think a new tagged release will be helpful as well.

This is just a core version requirement bump, so it could be committed. However I think the value of this module has gone down now that drupal supports this out of the box by having the new annotation.

For this reason I suggest to make a drupal 11 compatible release and after that this module should either be unsupported or it should use the new format.
I would suggest the first.

It looks like the only thing that's needed here is the .info.yml update, so this should be stress-free to commit. It would be great to also get a new tagged release after the commit.

I agree with @liampower. This looks good to go.

This is just a .info.yml update, so it should be stress-free to commit.

I also see this issue every time I am installing a site with this module on it. I can fix this by running a database-import. I think we can redefine the const in the module or put an `if (!defined()` guard around it.

In the proposed info.yml change there would be support for drupal 8.8 all the way through to 11.x, I guess having only 10 and 11 is enough though? I think this can be committed and a new release can be tagged?

Since this is only a .info.yml change I guess this can be committed and a new release can be tagged. This should make upgrading easier.

The change record looks good, I think this is good to go. I've removed the tags, as they were not the right ones for this issue.

I'm going to guess that the issue by @soutams has been fixed in the last year, and I would suggest to close this issue.

While I could see it being valuable to help in debugging the problem, I think the chosen route here makes sense and for that reason I think the behavior should stay as-is. I suggest closing this issue as won't fix.

Hello @apaderno, is there a reason why we're waiting for the stable release still? Can there be a stable release?

bramdriesen → credited borisson_ → .

I don't know anything about combing facets, is this something that is built in or with another module? I think combination of values should happen at the Search API level?

I may be missing something, but I'd like to get more info

pjonckiere → credited borisson_ → .

This works now, Thanks to @fjgarlin.

borisson_ → created an issue.

Thanks @knurg!

Looks like the latest merge request broke all tests, because of a missing ".ignored-deprecations.txt", I don't think this is because of this MR though. I wonder if this is something more general that's going on. I asked in #gitlab on Slack.

I guess we already have a test, so we're no longer waiting for that, we should make this into a merge request to see if the test actually passes.

This patch really is essential for nearly all my systems and I am really tired to apply it every time. Can we please add it? it works great!

@knurg, this issue is in the "needs work" status, for it to be committed, it should at least get to the "Reviewed and Tested by the Community" status.

A comment like the ones in #42, #43 and #44 could also put this into rtbc, if you feel safe enough that this will not break any other installations of the module.

So in this case the next steps are:

• Make the patch into a merge Request
• Get a code review on the code and tests
• Get this into RTBC state

Not a big deal but why is BEF here? Does it have anything specifically to do with Facets?

Since facets 3.x, the rendering of facets is done by views filters (instead of writing our own rendering/ajax handling), BEF makes those filters more capable.

borisson_ → created an issue.

quietone → credited borisson_ → .

I think this issue was supposed to be marked as fixed?

I think this is all the test coverage we need. Looks good to me.

It's 4 years ago, I'm assuming no one will add anything to this one, there is no log of the meeting here, but going back this far in slack will not add any value. So I'm going to go ahead and mark this as fixed.

I have to be honest, I don't really remember all of that conversation, but I hope @webflo does.

pjonckiere → credited borisson_ → .

This is a dependency already. I can't reproduce this.

I've now read the code, and tested it locally as well. I have one question, but I think it looks ok so leaving at rtbc.

I think it would be a good idea to add this to the next release as well, so that hopefully drupal cms will start with this new more better user experience.

So this change does not need a new config action, it only needs this change and the config action/recipe will set it to the new :all selection?

I think this is a good solution and I can't find anything to complain about really, apart from there not being any test coverage.

At the least I think we should add to the RenderedItemTest.

borisson_ → created an issue.

The most important IMHO is to prevent search_api_views_data to put a lock on cache_config that would prevent drush cr from correctly rebuilding caches.

I talked to @webflo today about this issue. We think that putting a lock, in addition to removing the return is probably a really good way to ensure that we always have a valid views defnition.

We never want to have a silently broken site.

I haven't looked at the code yet, but the screencast here looks super impressive, I think this is a huge upgrade.

Haven't tested it out locally, but the code changes look great.

If a field type is layout_section, the value always seems to be "Text" instead of the actual text I filled in in the layout builder content. That gets sent to the AI, but a new layout section does not get created either.

It seems like there are multiple things that can still be improved here.

array:1 [▼
  0 => array:5 [▼
    "delta" => "850c5e44-8e63-4435-a015-2556aa8c323d"
    "field_name" => "info"
    "field_type" => "string"
    "value" => "Text"
    "parents" => array:2 [▼
      0 => "info"
      1 => 0
    ]
  ]
]

I have attempted to test this on a local install, and it looks to me like this doesn't work yet.
The problem I encounter is that only the title is translated, and nothing that was placed trough layout builder.

To me it seems like something that needs to be changed in the TextExtractor. Will investigate further.

borisson_ → created an issue.

borisson_ → created an issue.