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

Merge Requests

More

Recent comments

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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?

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

This looks great, thanks for cleaning this up.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

Actually I think this can go back to rtbc

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

ExtentionAvailable makes a lot of sense to me.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

Back to rtbc now that it is moved.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

Removing the tag, title looks great now.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

Add belgium

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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?

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

This needs another rebase.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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?

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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.

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

This should be a merge request, not a patch?

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

🇧🇪Belgium borisson_ Mechelen, 🇧🇪

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

Production build 0.71.5 2024