Mechelen, 🇧🇪
Account created on 22 November 2012, over 12 years ago
#

Merge Requests

More

Recent comments

🇧🇪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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

I think the changes in the MR look good.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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?

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

All the comments are in there, looks good.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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...

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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?

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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?

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

This looks great, thanks for answering that question.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

2 additional remarks.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

Looks like all the remarks by longwave have been resolved.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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?

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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?

🇧🇪Belgium borisson_ Mechelen, 🇧🇪
// 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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

The update path should be moving none to null?

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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?

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

All feedback that I had suggested is now fixed.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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?

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

borisson_ created an issue.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

borisson_ created an issue.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

Production build 0.71.5 2024