🇧🇪Belgium @borisson_

I think this might work for some entity types, but not for all, one of the errors I see in the tests is this one: user_id.0.target_id=This·entity·(user·-·User:·2)·cannot·be·referenced.||flvpwaxl.0.format=The·value·you·selected·is·not·a·valid·choice.'

This reference (user - User: 2) doesn't seem to add any value and I think only adds confusion. At the very least this needs to be fixed so that users don't have the double reference.

For paragraphs, I see value in this.

Looks good and we have the phpcs rule in both branches as well, and the 11.x version is already in. RTBC

Including this as a coding standards rule makes sense, and to me it doesn't need to run through the Coding Standards Committee, because it's a language-level decision and there's nothing we can change or document about it.

Since this passes the new rule, we can be sure we caught all the changes.

I think this looks great. I agree with @quietone that i'd prefer to see this go in when we already have a rule.
However since there's not a phpstan rule for this yet and it seems like it's not too many changes for me this seems like it is good to go.
It's an improvement of the current status.

I think the "controversial" point is not controversial at all and I've also verified it can only return false.

I wondered if we didn't have to replace it with anything else, but the commit linked here already mentions that it was already not working since php 8.0.

#[\Deprecated(since: '8.5', message: "as it has no effect since PHP 8.0")]

This looks good to me.

Back to rtbc, this is indeed a better solution.

This looks good to me, this adds the needed test coverage.

I think it's a good idea to do, but if we can create a rector rule, to automate the refactoring, it doesn't have to happen at the same time.

There are still several test failures in the latest test-run, will those all be fixed in this issue or in several other issues?
I would suggest doing it all in this issue, since that would reduce the number of needed rebases for the other patches in the queue

This looks good, thanks.

This merge request looks great and I think we can merge this. Since the discovery of normal annotations is not yet disabled, this shouldn't impact any plugins out in the wild.

This greatly improves the ability to debug these errors.

The patch in #54 does not reflect the current merge request, also making this no longer assigned to @mahaveer003, since they are no longer working on it, it seems.

Looks good to me.

Should the views schema definition go in the facets_exposed_filters module?

Good question, I think it should, that makes more sense to me.

I had a coding standards nitpick, otherwise this looks great.

I think this description makes sense, looks good to me.

It should be rescoped based on #5, yes.

Composer 2 is now a required part of installing drupal. So I think we can use that information in installed.json instead of doing all the logic in this patch?

I don't agree with the route taken in the current MR as well. It seems like this is very small scope, instead of hard checking against the string type can we check that it's not a type that's iterable, such as map or sequence?

Needs work based on #34

Since all the issues linked in this meta are under development, I think we can close this one.

I am not too sure about this either. The patch we have today is an improvement over the current state, because we at least explain some things. I think we should try to find a way to properly validate this further. I think we need the followup to find a way to validate it strictly.

Not sure I see many issues land that are only for contrib and not useable by core.

That's true, there's usually the rule that there needs to be at least one usecase in core. But I think that's mostly about code-implementations.
This will help for core development as well, since it will help everyone working with this that is using an IDE with code completion.

In hopes not to derail this issue too much, I wonder if it's a good plan to start with one random thing that returns an array as we're doing here or if we should form a plan first.

Back to rtbc.
Since this is not a final class I guess the use of a private function was not really appropriate to start with?

How is this intended to run? Inside packaging of a deployment artifact for end users?

I have been thinking about this for a while, I'd like to think we can make this a dependency of core-recommended and not include it in core-dev. That way it can be a post-install script, however you are right there are some base classes we would need to keep around.

@michael.acampora worked in the related issue on similar things, that issue is now closed as duplicate so crediting here.

Closing as a duplicate. Will credit michael.acampora in the other issue.

This is missing instances in:

1. core/modules/search/config/schema/search.schema.yml
2. user.flood in core/modules/user/config/schema/user.schema.yml
3. core/modules/dblog/config/schema/dblog.schema.yml
4. core/modules/navigation/config/schema/navigation.schema.yml
5. core/modules/announcements_feed/config/schema/announcements_feed.schema.yml
6. core/modules/automated_cron/config/schema/automated_cron.schema.yml
7. core/modules/system/tests/modules/icon_test/config/schema/icon_test.schema.yml

I used a very inaccurate grep to find these: rg --multiline --multiline-dotall 'Range:(.*)min: 0', but I manually checked these and those should be fixed as well. Please make sure to check all schema files.

Tagging this issue as novice, we already have test coverage so all that really needs to change is in the config schemas for these changes.

Looks complete

quietone → credited borisson_ → .

The link in #2 no longer works.
I also tend to agree with @joachim in #11, single use providers can easily go to provide for multiple test methods.

I also just commented this in the slack discussion:

I don't think the names have to match the test names. I have gotten into the habit of doing something like provideMenuLinks - a name that is based on what is being returned. If there are multiple needs it could be provideInvalidMenuLinks/provideValidMenuLinks or even provideMenuLinksAllUpperCase.
I agree with the having provide or provider in the method names, and I personally prefer the earlier, because that way we kind of force people to come up with a name that means something?

#6:

We don't enforce method names anywhere else, so I don't think enforcing one for data providers is appropriate

I mean you're right about this, it's not in the coding standards, but all test methods start with test (public function testSomething). It's also possible to use another name and annotate with @test, but I don't think we do that anywhere? At least I couldn't find something.

These changes look good, in my opinion they are RTBC.

Credit looks correct and this seems complete. RTBC.

Actually we don't have any rules about this, but I think I prefer PositiveOrZero, just because it's more expressive compared to Range: min: 0.

If I am not mistaken, https://phpstan.org/user-guide/rule-levels, the actual checking of the array shapes' contents will start from PHPStan level 5 above; we are currently struggling with level 1...

This is already noted in the Issue Summary as well. I agree - I don't think we can start doing this without us being on level 5 as well, because the arguments in #7 need level 5 to be valid.

Changing status, because it's clear there's no consensus at this time.

That message from @rfay seems like a good reason to won't fix this issue.

I'm unsure about this, while we do allow phpstan types to be added in code since a recent coding standards update, this would be the first time we'd be adding a type like this as far as I know.

I'm not sure it was the intention of the coding standards committee to allow us to introduce a typing as complex as this one.

How do we make sure we keep the type compatible with the actual array structure, can phpstan help us with this or is that something we'd need to define in a test?
Should we not bother?

I do see the value in doing this though, it makes IDE's so much happier.

I agree we need to fix these independent from the other issue and with at least one test not actually running this seems like it's at least major.

I would've expected to only see file and class name changes, but there's a couple of extra things as well, mostly removing phpunit annotations and adding new attributes instead. Is this part of the transition to phpunit 11 and were these files missed because they weren't discovered or why are we including these changes?

I haven't used this yet either, but I think that committing this as is is a good idea now that drupal has standardized on ddev.

esmoves → credited borisson_ → .

I agree with the second one; that sounds useful. The first one however (index items) seems like it shouldn't be happening at all?

I think the new navigation is looking a lot better, to me this looks good to commit.

borisson_ → changed the visibility of the branch 8.0.x to hidden.

quietone → credited borisson_ → .

I think this is mostly already done by the field mentioned by @joachim, so I think we can close this issue.

esmoves → credited borisson_ → .

Instead of putting this on postponed, indicating this might happen someday, I will close this instead.

I think the changes in the MR look good.

Agreeing with @sokru that there needs to be consideration when adding additional fields to search api's admin screens.

I'd like to think this is mostly resolved now, we should probably check if all the page have been converted? Have there been edits in the time since we created this?

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?