🇧🇪Belgium @borisson_

I think this issue could use a title update, from the title it is not very clear what happened, I tried to change it so it's clearer.

#4: I used an online html to markdown provider. I did the rest by hand, so there's no script to upload.

All the comments are in there, looks good.

quietone → credited borisson_ → .

Have a look at 📌 Allow usage of stdclass instead of object where it makes sense Needs work for object vs stdClass

I have now taken the time to read most of this code. This is mostly new code, so converting it to the 8.x branch shouldn't be too much work I think. I had some small remarks, that I've posted on the MR.

I think the usecase for a shared base module for lucene data providers would reduce the amount of code here as well - but that seems like it would not have to stop this issue, since it's a much larger scope.

This looks like a super small change, I think this might need extra tests as well though?

Changing issue status to be needs review though.

Can we add the Search API Processor to getDiscouragedProcessors on the backend? This will give it a warning in Search API's view, so it's clearer.

Search API solr does similar things for example for the stemmer, which should also be discouraged here, https://git.drupalcode.org/project/search_api_solr/-/blob/4.x/src/Plugin...

Looking at the error, it seems that search api db has already made the change to add a return type to setUp. This change is not yet done here?

Since composer is the only recommended way to install can we close this?

As long as it's not the only possible way to install modules, there's still value here I think.

Voting for Marine Gandy (Mupsi)

In Search API terms it allows for exposing any kind of data, in Search API solr it can be used for both usecases:

1. Indexing items from multiple drupal site, and reading from it in several other drupal sites without them having to know the fields.
2. Reading data that is indexed by other systems.

So it's both the usecases you have described that can be made possible with this.

So far, this is working beautifully. What extra things do we need to do to get this committed?

In attempting to try this out, I noticed it only applies to the 7.x branch, and not to the newer 8.x branch. Which is understandable, trying this out on 7.x now

esmoves → credited borisson_ → .

I can't figure out how to push to this repo. But I have a patch attached.

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

borisson_ → created an issue. See original summary → .

Looks correct.

Ok, I hope I didn't miss anyone in this list, marking as fixed. Thanks everyone!

borisson_ → created an issue. See original summary → .

Ok, I double check that as well, makes sense. All my questions have now been answered.

Tests are green, that's great! I think the new constraint also makes a lot of sense.
There's an upgrade path and the changes to tests also look good.

This looks great, I have no additional remarks on this.

but i did need to update a theme test that was loading a module from a profile that is not installed.

Does this signal the need for an upgrade path, or is this because we do weird things with the testing profile?

quietone → credited borisson_ → .

Asked for more clarification in the MR comment (and now posted here as well, so that this issue goes back to unread).

Looks great!

There seem to be a lot of unrelated changes in the MR. But the latest commit looks great.

I think it's better to stricter if possible in this case, since it allows for less discussion and I think that's why we have rules. Not having a rule is the worst possible solution in my opinion.

I would prefer to close this issue as won't fix.

I agree, so let's change the config schema to use minimum values of 0?

Looks good, thanks!

This looks great, thanks for answering that question.

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.