Mechelen, 🇧🇪
Account created on 22 November 2012, almost 13 years ago
#

Merge Requests

More

Recent comments

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

I think having string keys for data providers seem like a good thing to have, but thinking about it some more, it does seem like it would not be easy to do as a coding standard. I am not sure we can always require this. So documenting it in the Test coding standards seems like a good path forward.

For this issue, I am a bit unsure what the overlap is with 📌 [policy] Remove the requirement for doxygen for test methods Needs work , since that already takes care of the requirements for documentation. I think that means we should close this issue?

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

The text in the MR seems clear to me to understand the rule and intent to still document where it is needed.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

These changes already look great, let's do the simpletest one in a followup.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

I think the wording here makes sense, adding myself as supporter as well. I think we have enough supporters if we count the older comments on this issue.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

> However, if we mandate that every set of arguments is keyed by a meaningful string then this is super useful.

That is super useful, but it's code, not documentation. Out of scope for this issue?

I think this is a very useful thing to do, without it being the same as 📌 [policy] Remove the requirement for doxygen for test methods Needs work , can we use this issue for adding that requirement?

🇧🇪Belgium borisson_ Mechelen, 🇧🇪
🇧🇪Belgium borisson_ Mechelen, 🇧🇪

I think this looks good, the documentation here looks great and the code itself looks clear.

I think we should add a followup issue for api.d.o to handle `@template` in a good way.

Additionally, we don't usually add new functionality without implementing it somewhere, and we can't really add this to any existing api's without breaking backwards compatibility. I wonder where/how we're going to implement this, is there a followup already created where we know where can implement this?

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

I talked about this with Matt Glaman at drupalcon last week. I'm hoping he can find a good solution for this, while a plan would be nice, I think we can start small and start adding it in places where we can add value.

I just really don't want to add this without it being validated, but it sounded like Matt had a good solution for that.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

The CRs look really good and they seem easy to read. Should we mention in the recipe.yml translation CR what properties are automatically extracted by potx, so people don't needlessly add !translate to them?

Am I understanding it correctly that we could now add other yaml tags, that can be casted into a different stringable object as well? I can't immediately think of a good usecase, but that seems like a powerful thing to do.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪
🇧🇪Belgium borisson_ Mechelen, 🇧🇪

borisson_ created an issue.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

The last 2 commits look great, and it was previously rtbc, so restoring to that state.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

Ok, so I misunderstood how it works. I think this means we need to follow @alexpott's advice and we should close this issue in favor of a phpstan rule.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

I discussed many things with many people

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

This looks good, I read this page, as well as most of the page from the fiscal host. I think we can go ahead with this. I will discuss with Sven if we can be one of the admins.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

I talked with @alexpott and @Wim Leers at drupalcon about this.

Config validation does not run during normal operation of a site. And these checks are also replicated in the installer.
When using a normal workflow to do imports and exports of config, the importer already has several special checks for the core.extensions and it will already yell if a file is invalid.

This is replicating a lot of that logic (but not all).

If you can point us to a place where the validation happens during import/normal runtime that affects this - please set this back to needs work. I think all the other points in this MR have been answered so putting it back in rtbc.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪
🇧🇪Belgium borisson_ Mechelen, 🇧🇪

I took pictures

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

I took pictures

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

I took pictures

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

I took pictures

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

The usecase for facets is no longer there, since we're doing this way differently now. Removing the tag.
I haven't tested it again either.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪
🇧🇪Belgium borisson_ Mechelen, 🇧🇪

borisson_ created an issue.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

I agree with @acbramley, I think this can be closed as outdated because that new error seems like it makes a lot of sense.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

Tests seem to be failing - but I hope this is a random thing? The changes here look good. It makes sense to enable to rule now already.

+1 to rtbc when the MR is green.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

Doesn't look like it should require tests, I agree with this change. RTBC++

🇧🇪Belgium borisson_ Mechelen, 🇧🇪
🇧🇪Belgium borisson_ Mechelen, 🇧🇪

In my mind, the main use case is actually the one from the change record: You’re already checking for, e.g., $index->hasValidTracker() but then the IDE still marks your call to $index->getTrackerInstance() as potentially throwing an exception.

This is a very valid usecase indeed. I agree that will make it easier and since this is such a low impact bug-wise, while still increasing the DX, it's probably worth getting this in.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

I am personally not a huge fan of having to do NULL-checks (or null-safe chaining) instead and this increases our api surface by quite a bit.
The chance this is introducing bugs is relatively low though, so not opposed to this based on that.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

This is still a useful thing to do, we're not using these. However, we now have a deprecation policy that we should follow for this.

\Drupal\Core\Cache\Context\CacheContextsManager::getLabels should probably also be deprecated?

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

I would very much like to see the other issue go in instead, this should be a boolean.
It also indicates that we're missing some kind of testcoverage, since the steps to reproduce are very easy.

However, to me it seems like this is a regression that we can easily fix in this issue, because the other issue has been going for a very long time and I don't see that landing quickly.

Adding the right tags to this issue to categorize it in config schema validation part.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

This change looks good, RTBC++

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

Looks good to me, I think we can commit this as-is.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

I think this still makes sense to do. To me it seems like the next steps are rerolling and adding a merge request + improving docs. Tagging as novice to do that.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

This makes a lot of sense, but it seems like there's still some tests that need fixing.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

It makes sense to do this already, we allow to use phpstan types in phpdoc.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

To me, it seems like this is sufficient test coverage

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

I think a lot of the places where more than one constraint is defined we should use this. Should we pick one or perhaps just do follow ups? What do you think.

I think doing followups makes most sense here.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

While this is great, we don't usually add new features if they don't have an immediate usecase in core. Do you know where we could be using this? Or is this just about exposing all the symfony constraints?

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

I have one question on the MR.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

> I think, asssuming we use markdown, this could just mean move through history, copy html, convert to markdown, add proper commit message. Next.

This could be an option, but that seems like a lot of work - it would mean we can delete all these pages easily from d.o, so that has my preference as well. I hope this can be scripted, because there are a lot of revisions on some of those pages.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪
🇧🇪Belgium borisson_ Mechelen, 🇧🇪
🇧🇪Belgium borisson_ Mechelen, 🇧🇪

I agree, I also prefer to always use the extended format.

Added template

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

I believe it is still relevant - though I am personally not a fan, but without a coding standards issue we shouldn't get this in. I think we need to create a CS issue and postpone this one on that.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

No, that doesn't seem related, this looks good to me.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

This looks great! We've been using this patch in production for all this time and it's been working great for us so far.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

I'm not super familiar with the new contribution records yet, but it seems like no one has gotten credit for this issue yet?

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

Back to rtbc, this is indeed a better solution.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

This looks good, thanks.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

This greatly improves the ability to debug these errors.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

Should the views schema definition go in the facets_exposed_filters module?

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

It should be rescoped based on #5, yes.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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?

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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?

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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?

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

Credit looks correct and this seems complete. RTBC.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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?

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

Production build 0.71.5 2024