🇧🇪Belgium @borisson_

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.

borisson_ → created an issue.

borisson_ → created an issue.

Looks super simple in code, I think we should just do this.

I have prepared the first MR for recipe integration https://www.drupal.org/project/search_api/issues/3484304 ✨ Expose Index and Server methods as config actions Active . It exposes only already existing methods as config actions to start this feature rolling.

This is a good and easy first step, I think that one is ok.

• addDatasource, but at the moment the argument is of type Datasource and it can't be used in recipe. Maybe alternatively there could be a method addDatasourceByIdAndConfig that will have datasource id and its config as arguments.
• setTracker has and object as argument so it can't be used as action method, but maybe having a method setTrackerByIdAndConfig with tracker plugin id and config as arguments would make sense
• The same for setServer it makes sense to create setServerById?
• addProcessor has object as argument, but maybe new method addProcessorByIdAndConfig would be ok to add as well
• addField has Field object as argument, but maybe new method addFieldByFieldConfig would be ok to add

For datasource, and server I think adding a new method is a good idea.
Processors and fields are more difficult, because their configuration can be more extensive, but I guess we can go through the same way here as well?

I think tracker is probably not needed, since for 99% the basic tracker is used. So I'm not sure how I feel about creating this new method, however if we already do this same trick for the other 4 I guess it's not too bad?

if it is already comitted, I can set this to fixed.

borisson_ → created an issue.

I think we can still work on this issue, or if we move to another issue we need to credit the people who helped already here. I think it's a safe move to reuse this issue.

breidert → credited borisson_ → .

greggles → credited borisson_ → .

Kickoff meeting for event in 2025

borisson_ → created an issue.

Was fixed in 📌 Add config validation for weights (blocks, filters, etc. all use weights) Fixed

borisson_ → created an issue.

I participated also

I took pictures

I took pictures

I took pictures

I took pictures

Updated the summary, thanks!

I'm not sure we want to add a config schema for views plugins at this time, core views are not yet validated either yet.

esmoves → credited borisson_ → .

larowlan → credited borisson_ → .

wouters_f → credited borisson_ → .

Not sure what this tag was.

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

I feel that ExtensionExists should be whether the extension exists and we should have a separate constraints - the other constraint should be called ExtensionInstalled. However this is unfortunate because we already have ExtensionExists and it's current meaning is that it is installed.

Not sure what to do.

This is a difficult question. In my opinion ExtensionExists/ExtensionInstalled makes sense, but renaming this and after that reusing the name for the other meaning is difficult. Can we use ExtensionPresent to indicate that should be present on the file system?

Committed, thanks!

I created a new MR that includes all fixes, also I replaced str_contains with strpos,
because not all websites support this (str_contains only exists in php8)

Hard disagree. Please use str_contains instead. https://github.com/symfony/polyfill-php80 will ensure everyone can use this.

hestenet → credited borisson_ → .

Created a release for 2.x

So it was released less than 24 hours before this issue, that's certainly very quick.

I'm not 100% sure, I think the current 2.x branch already supports drupal 11? If that is the case I can tag a new release soon.
I don't think there will be a release on the 3.x branch in the coming weeks, since that is currently still undergoing architectural changes.

We'll only tag a new release once we get to a more stable situation, this is still undergoing big changes as it is.

The earliest I see this happening is at drupalcon barcelona.

Commited and pushed to both 2.x and 3.x

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

This looks correct, the 3.x (and especially this submodule) is not yet stable, so we're going to add this once we've settled. Will keep this issue open to resolve that when the time is there.

I reviewed it again today as well, and the one remark I had was answered. RTBC++

larowlan → credited borisson_ → .

Committed and pushed to 2.x and 3.x, thanks everyone!

quietone → credited borisson_ → .

It will be hard to keep this in mind while reviewing config schema issues, but since it's a simple set of rules that are easily documented we should proceed like this, I think #6 tackles my concerns.

Most things in system.site that are being made nullable in 📌 Add validation constraints to system.site Needs work , are not actually nullable from the standpoint of a running drupal installation, this especially rings true for name and UUID.

However, I'm not so sure about others. What is the semantic meaning of page.403 / page.404 being an empty string. It means this is not configured, right? I think it makes more sense in that case for it being NULL, because we can say it has to be null or it has to be a valid path.

If it is not nullable, we can not be as strict on the shape of the value?

page:
  403: ''
  404: ''

page:
  403: null
  404: null

I think this will reduce the need for nullable and means that we can leave values as strings and not have type flip-flopping.

Type flip-flopping is not great. and it doesn't make our code more strict.
All it does, is allows us to assume if the value is a string, it's a valid value, but I would not like to see a lot of places introduce is_string as checks to see if it's the right value.

I don't yet have a fully formed opinion on what the best way forward is.

I've opened #3461428: Add NotBlankAfterInstall constraint and use it for system.date:timezone.default to explore the NotBlankAfterInstall idea.

I think this is a very valueable new constraint.

Commited and pushed to both 2.x and 3.x, thanks!

Merged, thanks so much.

We no longer have the array of ignore things in ChemaCheckTrait, as far as I can see, so I think you're right that this can be closed.

Looks great, thanks all.

Thanks, pushed and committed to 2.x, can we can get a 3.x variant of the change as well?

I fully agree with #11, I think that example makes sense.

Spellcheck fails, but not sure how to better name this.

dan2k3k4 → credited borisson_ → .

borisson_ → created an issue.

This is not the way, the way is in search api

Back to rtbc now that the remarks have been fixed.

This requires us to add something like EntityTypeExistsValidator, currently working on that.

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

Merged in these changes, should we open a new branch for 3.x?

This looks great. I really like the test coverage provided here. That looks like it covers all possible combinations. I think we should wait for 📌 Allow parsing and writing PHP constants and enums in YAML files Needs work to get in.

Hurrah, it's green.

I think the mr needs to be rerolled?

Followup created: 📌 Add PHP_INT_MAX to integer schema configuration Active

borisson_ → created an issue.

The remaining failures are all in rest, but I don't understand how to fix them.

Discussed with @alexpott at drupal dev days, setting default values on the entity.

We now have specific test coverage for a type that is null. This looks good to me.

Changes look good to me. RTBC.

I think this looks great, this makes it easier for other issues to land. Marking as rtbc.

Looks like a lot of the current failures are because of invalid configuration in tests.

This was fixed in another issue.

Committed to 2.x, not sure if we need this for 3.x also.

Thanks!

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

Committed to 2.x and 3.x, thanks!

Merged to both 2.x and 3.x, thanks!

borisson_ → changed the visibility of the branch 3342656-facets-searchbox-widget to active.