Massachusetts
Account created on 15 November 2007, over 16 years ago
  • Senior Software Engineer at AcquiaΒ 
#

Merge Requests

More

Recent comments

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

The problem and fix that Wim mentions in #8 are, in fact, the reason why this is critical. From a Slack thread:

Application fails if it runs into invalid config from contrib.

So we have to limit validation to only FullyValidatable config, pronto.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

This has turned out to be critical. See this Slack comment:

I install recipes in /recipes/contrib. If I apply a recipe that calls other recipes in the same folder, they are no longer found.

So the lack of this is breaking sites that are already using recipes. Big problem.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

You know, that's a good point...what if we changed this line in RecipeCommand:

    catch (InvalidConfigException $e) {
      $this->rollBackToCheckpoint($backup_checkpoint);
      throw $e;
    }

to this:

    catch (\Throwable $e) {
      $this->rollBackToCheckpoint($backup_checkpoint);
      throw $e;
    }

...and just let the plugin system do its thing? We could keep the expanded test coverage.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Fixed most of the feedback; I don't agree that an empty set of permissions is necessarily an error condition. It's nonsensical, but it's also harmless.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Okay, I'm struggling with how to test this.

With any fully validatable config, it's kind of impossible to put it into an invalid state. As soon as we save it, we validate it, and that throws an exception. How, therefore, can we contrive a situation where the imports and the actions (both of which validate everything they can) somehow fail to catch an invalid config object?

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Yeah, I agree with you, Wim. Postponing.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

This seems straightforward.

Unless I'm missing something, I don't think this could cause a BC break; anything in contrib that overrides this method is either keeping it protected (and which would be fixed by this), or have made it public, which should continue working after this is committed.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Well, I think that looks pretty good, at least from a code perspective!

I need to give it a manual test, but since this is a proof of concept, I think we could call this RTBC. It probably doesn't need to be committed right now, but it could be applied in conjunction with the recipe system (which is a core patch itself). I'll add this to my recipe-test repo, which sets up the recipe system with Project Browser.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Thanks for that screenshot, @srishtiiee, it looks good to me! It's a bit sad (and concerning) that none of the recipes in it are compatible -- are we sure that compatibility check is working? At the very least, I've been able to get kanopi/gin-admin-experience to work on Drupal 10.2.x.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

I have a couple of thoughts but I think that looks pretty good!

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Updated the issue summary.

I don't think we need to define a whole separate "Except" action, to be honest. I find it simpler and clearer to just have it be an option to the permissions_per_bundle plugin; making it a distinct action means you have to remember that if you want to use the except option, you have to use a different plugin ID. That's poor DX, and given that it adds to the complexity on our end, I don't see a reason to do it that way.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

As promised in #33.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Few nits, but otherwise I don't see any reason to hold this up!

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Liking the new test coverage! I have nits, and only one is blocking (an assert that doesn't actually assert anything). But otherwise, happy to restore RTBC once the one thing is fixed.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

I think this looks good. I do have one question on the MR for @tim.plunkett, though: https://git.drupalcode.org/project/project_browser/-/merge_requests/430#...

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Everything in here makes sense to me. The weird parts are well-explained and commented. I feel pretty good slapping the RTBC on this!

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Ideally we could remove the multiple is_array() calls - we shouldn't be changing runtime logic just to shut PHPStan up, if we can at all avoid it. Posted a suggestion, but if that won't work, we can keep it as-is.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Pushing back on one point, because it is a pain in the butt.

But otherwise, this feedback has been addressed, I think.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

I think all feedback is addressed; some threads have been left open for Wim to sign off on.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

#12: I think changing them here is out of scope. The plugins are not really named with clear casing; they generally have snake_case IDs (as do most plugins in core); they only seem to be camelCased if they are delegating directly to methods of config entities, which are all named in camelCase.

So, I think instantiate_on_all_bundles is probably the correct name for the plugin, since it is not delegating to a particular method of a config entity.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

This is looking much better, and clearer! I have some thoughts, mostly minor things, and we still need to cover a particular test case. Other than that I think I'm rapidly running out of complaints.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Updated the issue summary.

We no longer need to validate "before saving" -- that was written before we had the concept of rolling back to a pre-recipe backup state. We can validate after saving -- and indeed, we should, since config may be changed during save -- and let the recipe runner roll back if needed.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

This looks really good to me. Most of my remaining feedback is quite minor!

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

I have almost no complaints here. Just a couple of questions. But otherwise I feel like this is RTBC.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

This looks great to me. It does exactly what I imagined!

I think the code should be streamlined and simplified in some places but I don't have any serious concerns.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Responding to #5:

Based on reading the issue summary, I would also assume that field.storage.. already exists prior to applying the config action.

Yes. The config action should assume the storage exists already.

What if one of the field already exists?

Then the action shouldn't do anything, I think. But I agree with Wim's thoughts - we could parameterize that so that it fails if the field exists.

Updating the issue summary with these clarifications.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

I sketched in some basic test coverage that we could easily expand later to cover additional entity types, if we want to. The stuff I pushed covers the four entity types in #10.

So this just needs implementation; leaving that for someone else.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

I think it's ready for another look. All feedback resolved, except for one, which I'm not sure how to approach.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Okay, well, I pretty much reverted everything and just added those few search paths. Ready for another look, I think!

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

#3: Drupal looks for modules in those places, yes, but also in sites/*/modules and within the current installation profile. That said, we don't need to search for recipes in those places, necessarily. I'm on board with keeping it simple and doing what you propose, but IMHO it costs us very little to take advantage of Composer here, and it will allow us greater flexibility if some sites need to put recipes in different places for whatever reason. (I can totally imagine a case where site builders want certain recipes to be in sites/examples.com/recipes, rather than webroot/recipes.)

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Honestly, I'm not sure what else is needed here.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Self-assigning to update the issue summary.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

This is a subset of that issue, yes. I have to convert it to a meta.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

No need, @DamienMcKenna. If it needs to be changed (I suspect it will pass all validation), we can just change it.

Besides, this is ready to go, and #3400672: [PP-1] Robustly validate the structure of recipe.yml β†’ has quite a bit of work still needed. If you manually test the recipes in this issue, you'll see that they work as intended. :)

So, I see no compelling reason to block this.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

This stuff is really tricky to understand, so nice work @omkar.podey! That said, I did a code review and found some stuff that should probably be fixed, plus some questions that need answering.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Right you are about Taxonomy! I didn't even know it had adopted per-vocabulary permissions. Updated the issue summary with that.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

I noticed something about setting the summary -- if there's some logic around setting a property, then we probably don't want that property to be public.

Otherwise this looks great.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Oh, I love the way this is looking!

I think w can harden it somewhat, and add more test coverage to be sure this really works as designed. :) Commented with some ideas.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

I'm not aware of any other entity type besides node types and media types which have permissions like this. So I'd be okay with just implementing this as a plugin that is aware of those node types in particular, as long as it doesn't hard-code too many assumptions (i.e., in case we want to support other entity types from contrib that have a similar permission structure).

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

I think this is blocked by #3421197: It is impossible for recipes to depend on recipes outside of their own directory β†’ .

Without that issue, we cannot validate that recipes depend on other recipes that actually exist, which I think is a critically important guard rail we should implement.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

I think I'm pretty happy with the way this looks.

All tests are passing, and the issue summary is up to date. I think we're ready for review.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

I think this looks really good. Also, check out that diffstat:

+ 201
βˆ’ 568

That's nearly 400 lines removed! NICE!!

The only thing I'm unsure about here is the use of readonly -- that's changing the behavior of the Project class, and to me it should not be done in this issue.

But otherwise this looks RTBC to me.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

I think this is a fantastic start, and quite straightforward and generally easy to understand. There are some spots where it needs to be more robust and better-documented, and we need to expand the test coverage somewhat, but I think this is a very strong foundation upon which to iterate. Nice job, @srishtiiee!

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Just to note it in writing: I'm not marking the following field setting as fully validatable right now, because their supported default values (for example, IntegerItem's default min setting is '' (empty string), which is nonsense from a configuration standpoint) conflict with the logic in \Drupal\Core\Config\StorableConfigBase::castValue(), which specifically treats empty strings as NULL for integer and floats.

I'm not exactly sure what the correct fix for this is, but it's definitely not something to fix in this issue without blowing up the scope.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

My feeling is, if that happened, it would be a perfectly legitimate reason to open an issue to add the newly-supported letter. :) That'd be a one-line fix and probably wouldn't even need test coverage.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Wait a minute here. Might we not be massively overcomplicating this?

Right now, the constraint we have for the pattern is this:

        Regex:
          pattern: '/[aABcdDeFgGhHiIjlLmMnNoOpPrsStTuUvwWxXyYzZ]/'

In other words: "this string must contain, somewhere, at least one of the characters that we know is valid to use in a date format as of right now".

This regex has no opinion about unsupported characters, or escaping anything, or characters that might be supported in the future. All of those things would pass this regex. It simply says you must have at least one of the characters that is supported now.

So, unless PHP 8.4 drops support for all of these characters in date formats...this pattern will continue to be valid. And rightfully so. This is already a very permissive validation. Because of that, it's also very future-proof.

Therefore, I think #11 is maybe not looking at this correctly.

I'm restoring RTBC, but if I'm barking up the wrong tree, feel free to knock it back.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Update path and update path test added.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Oh, and about that update path...if we do it, we might want to do it in a separate issue, and have it ensure the langcode key exists in all simple config.

Otherwise we're almost guaranteed to run into this again as we make more and more of core's config schema-compliant, and then you gotta wonder if it's better to have lots of little update paths which all do the same thing (dear god, no)...or just have one big one that fixes this known schema compliance issue.

Production build https://api.contrib.social 0.61.6-2-g546bc20