🇧🇪Belgium @borisson_

2 additional remarks.

Looks like all the remarks by longwave have been resolved.

Updated the page title to be in line with most of the other issues in the validation space.

items_per_page and pagination_heading_level are removed from the views_pager_sql schema, but I don't see a reason for that in the issue, can you help me understand this?

There's an open question on the MR, can we answer that?

There is a note in the MR about not having to deprecate a method because it was not shipped in a release, since then it has though, so do we need to do a proper deprecation now?

// isAllowed - uses ip_window
      $number = $this->connection->select(static::TABLE_NAME, 'f')
        ->condition('event', $name)
        ->condition('identifier', $identifier)
        ->condition('timestamp', $this->time->getRequestTime() - $window, '>')

The ip window is used in isAllowed and register on the Flood backends, this is could be anything as long as it is positive, I would suggest this being greater than 10 to allow for time drift between different backends if needed, but this isn't techinically required. I also can't seem to find a form where this is configurable.

User window also doesn't have any actual minimum, but it should be greater than the value in ip_window. I think we should update the constraints based on what is actually happening in code.

The analysis in #4 looks great! That means we can do it easily with the same flexibility of constraint we have today. +1 to this approach

The update path should be moving none to null?

Validate level: must be between 0 and $this->menuTree->maxDepth()→ Range constraint?

Can a range constraint be variable like this, or should we fall back to the same thing with did for weight and say it is somewhere between 0 and PHP_INT_MAX?

All feedback that I had suggested is now fixed.

I think this is good, I can't find any changes I'd make to this issue anymore.

I think overall this looks great, I have a tiny documentation nitpick and a small change I'd like to see in the tests.

The changes that are done look good though, I think they all make sense.

The last run on the mr has a phpcs failure, let's fix that.

I agree with #97, I don't think we need an update path, but we do need to mention in the change record that saving after this update will trigger a lot of extra exports.

It's good to see that you already removed the novice tag again @bbrala, because this doesn't look very easy.

RTBC++, I agree this looks great.

I wonder how we work around BC here? This would be the first contraint we'd rename, so we need to figure out our deprecation strategy I think?

I agree, this is a lot better. RTBC++

borisson_ → created an issue.

borisson_ → created an issue.

I think the needs review queue initiative has made this a lot better as well, there are currently 177 issues in needs review for modern Drupal. So I think adding more people to that initiative is a better solution to this problem.

CR looks great.

Can we change this to a pull request instead of a patch? I also saw a mistake already in the first file's changes, so that should be improved.

Good idea, I think this is something we should offer, but preferable behind a feature flag?

These aren't all just php8.3 compatibility changes? Only the string cast is?

I agree with catch, we don't document all the negative examples and I don't think we should start doing that. We already have a lot of documentation.

Adding validation tag

bramdriesen → credited borisson_ → .

Add new types?

type: directory
type: file_uri

Yes, those 2 new types should be made, and those types should get a constraint.
For file_uri it could be a file_exists check (or that symfony constraint I guess). For directory it should either exist or be in a writable place?

So the question on the MR is if we can split out a config schema for a file vs a directory. I think that is always different. I don't know of any places in core (or contrib) where you have to enter a path to a directory-or-file. So splitting these into two different schemas makes sense to me.

While I think it has a low priority, I think it still makes sense to validate that the schema of translated items matches the original type.
I think this issue should stay open.

I would like to get this in as quickly as possible, so we don't see this style be used more often.
There's no phpcs rule for this yet, but if we agree on this, I will create an issue there.

Can the Benefits section be updated. For myself, I do not see the benefit of having 3 constants lowercase and all the others uppercase.

I think the idea is to have all the language-native constants be lowercase, in most editors they get their own highlighting anyway, since they are language constructs.
I think the changes needed to get this done (huge patches to core, all of contrib) don't outweigh the benefits.

I still would like to get a release out for this module yeah.

After asking in slack it looks like we could document this over at https://www.drupal.org/docs/drupal-apis/configuration-api/configuration-... →

I think we can document it there.

The suggestion in #53 makes complete sense to me, it's now updated so moving it back to rtbc.

I pinged kristiaan in slack, and in the meanwhile I figured there may be something else that we could do.
Instead of doing a min check here, we just need to know that the value is > 0, because smaller is not an option.

So I think isPositive might be a simpler way to validate this instead of being this specific.
However, waiting to put this back to needs work based on feedback.

I have one tiny nitpick on the comment, but I think @bbrala is right, this looks ready.

I think this looks good, I don't think we need specific test coverage for these values.

I wonder if we should ask for a user module maintainer for a review? Will ping kristiaan.

This looks great, thanks for cleaning this up.

The patches in #30 and #29 seem to be identical to #26.

I am not sure if the solution in there is actually a good one. There are good steps to reproduce, but I would like to get input from one of the config or entity system maintainers on the direction before writing tests.

borisson_ → created an issue.

Merged the MR.

The latest version of the patch is to disable the option in the views UI, but the issue summary doesn't yet reflect that. So that would be the first thing to do here. As a next step we should transform the patch into a merge request.

Actually I think this can go back to rtbc

ExtentionAvailable makes a lot of sense to me.

Back to rtbc now that it is moved.

While I agree with #5 there would not be a big maintenance burden, in the 5 years since this comment was posted there did not seem to be a big interest by others. I would live to see popularity numbers from this based on a contrib module and we could use that to inform the need for this, because to me it still seems like a super small usecase.

I will close this as won't fix for now.

The change record that is linked to this issue is not correct it seems?

I too am constantly bouncing between the need to validate everything strictly and the looseness that is allowed currently.

Can we do this the same way we define plugins? Each one having their own schema?

I reviewed the CR. It has all the information needed.

I have some small documentation remarks, but this looks great.

Still needs tests, the remark in #11 seems to be fixed.

Olivero was specifically designed like this, see 📌 Olivero: Center align content (instead of left align) Needs work for people who want to have a centered layout. But the drops background + left alignment is a feature, not a bug.

There are more tests that are failing on the last test run. All in ChainedFastBackend.

The change however makes sense and it has good test coverage.

bramdriesen → credited borisson_ → .

The parent issue was committed.

Since it's about removing a role, I'm not sure if we need to have strict validation.
However it could lead to an action that doesn't do anything when it has a role configured that does not exist I guess.

This looks great and it makes this test a bit faster.

Removing the tag, title looks great now.

Thanks for that information. Moving to rtbc based on #10.

Add belgium

There's a test failure in AssetAggregationAcrossPagesTest, but I've seen that fail in a couple other merge requests today. I'm thinking it is not related. The only bytes we are saving are from the comment not being there anymore.
I agree keeping the class around though, otherwise this would be a breaking change I think.

It is green now, and we're decreasing the page weight, that's great.
I am not sure about moving the file though, people are not supposed to reference the file by path, but it seems like it increases the likelihood this will be a breaking change?

Removing tags that are no longer relevant.
The new solution is well documented, but I'm not sure about the tradeoffs here, tagged for product manager review because of that.

I don't see why that test would start failing, and I can't find an issue about it having random failures. Since this a security hardening I think we should move this through.

nod_ → credited borisson_ → .

So, at the very least this issue sounds premature until the Drupal CMS being stable with a good search_api recipe is implemented. At that point, it sounds like the only concern is around user confusion?

This is where we are now, Drupal CMS 1.0 was released and it includes Search API with a good setup.
I'm not sure how much continued adaption it requires from people building on top of Drupal CMS, but I hope it is minimal.

I'm not sure about this. Administer Search is from the core search module. Search API doesn't have that permission, but it does have a Search index view mode as well.

How would we, as search API maintainers, allow this to work without asking people to enable core search, just to get that permission.

These changes look good. It also has the change to phpcs.xml.dist to prevent regressions.

bramdriesen → credited borisson_ → .

All threads seem to be complete. Looks good to me.

quietone → credited borisson_ → .

I agree with catch in #19, moving this to Drupal\Core\Config seems like a better solution, let's do that.

I don't understand having to bypass NotNullConstraint? I think we have to reimplement it in here?

We should probably start with a reroll here.

This needs another rebase.

I have gotten in the habit of using the markdown-like backticks when having to say that something is `code`. I think a lot of people will recognize this. Is that an option instead of using quotes?

We could even update the api.do parser to wrap those in a code-block as a next step?

Since this is only for the ajax behavior of the regular blocks, and we are currently only supporting ajax-like behavior through facets 3.x, I'm going to close this as won't fix.

The current suggestion is to try using views' ajax handling with facets 3.x for this.

It could be that this is also a bug on core views, but that would need a core issue to fix.

Only 2 instances, those look good. Back to rtbc.

I am not sure why this was suggested, but I don't think we need to check this? Any module could provide a plugin?

Title fix

While I agree that adding more functional tests is not great for the total test time duration, having them well documented is important.
We can always change the documentation when we want to add more testing to this.

To me, this change seems like it answers the question by @nod_.

Test looks solid, I think that's good. I wonder if we need a change record for this, but I don't think we do. RTBC++

This should be a merge request, not a patch?

I agree, this now no longer decreases readability, I think it does read better now.

This looks great and it has the phpcs change need to ensure we don't regress.

Found some places where we could align better, I personally really dislike things that end up looking like this: ]));

I'm not sure how facets should support this? As long as Search API gives us this as a string we can work with it. So I guess this is supposed to be in the realname project to support Search API