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

This isn't ready - several jobs are failing with a warning, which is still a failure. Facets itself is also not D11 compatible yet, so this can't really go before it does.

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

phenaproxima β†’ made their first commit to this issue’s fork.

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

phenaproxima β†’ made their first commit to this issue’s fork.

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

The PHPCS, spell check, and eslint jobs are failing. Can those be fixed before I merge this?

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

Got an initial implementation up with some basic test coverage.

This only allows getting input from an existing config value (we can add prompting and such in another issue). It supports reusing inputs from recipes you depend on.

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

Sure, it knows what will happened (well, actually what's already happened over in the staging area where Composer operations are done), but Project Browser's UI and backend controllers are not currently built for displaying a confirmation before the sandbox is copied to the live site. It's a Project Browser problem, not a Package Manager problem.

That's why I said it would need substantial refactoring. :)

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

Re #7: I agree, but that's not something Package Manager is willing to do :) If you change the path repos in composer.json to use absolute paths instead of relative paths, that would probably fix the problem without needing to change that Package Manager setting.

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

This patch is wrong as hell. The bot's broken, and besides, we're already compatible with Drupal 11.

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

What #10 suggests could be done, but would take some significant refactoring. We'd probably need to introduce a new method to InstallerController which evaluates the state of the sandbox (staging area) and reports back, before allowing the user to confirm what's going to be done.

That also means we'd be implicitly moving to a "shopping cart" type of user experience, where you add a bunch of packages to your sandbox, then review what you did before you apply the changes to the live site. That's a pretty big change.

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

That said, we are doing some kind of testing of it, so let's keep it for now but wrap it in an if check.

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

Yeah -- that line needs to be axed, frankly. Should have been removed when the real endpoint was promoted to a full source!

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

This is because Package Manager ignores any unrecognized files/dirs in the project root.

The workaround is to set package_manager.settings:include_unknown_files_in_project_root (config flag) to true.

πŸ“Œ | OpenAPI | Add Gitlab CI
πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

This is already happening in πŸ“Œ Automated Drupal 11 compatibility fixes for openapi Needs work , which is far along, so closing as a duplicate and transferring credit.

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

This is a duplicate of πŸ“Œ Automated Drupal 11 compatibility fixes for openapi Needs work , which is further along. Closing.

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

phenaproxima β†’ made their first commit to this issue’s fork.

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

All set here; merged into 8.x-1.x. Thanks!

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

phenaproxima β†’ made their first commit to this issue’s fork.

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

I cleaned up some coding standards stuff and got things nice and clean on CI. That said, we cannot truly test for Drupal 11 compatibility because OpenAPI UI, which is an implicit dependency, doesn't declare compatibility with it yet. Nonetheless, this gets us part of the way there.

Leaving in "needs work" for my review comments.

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

Still needs a little fine-tuning to prevent BC breaks. Also, the PHPCS and PHPStan jobs are failing in the GitLab CI pipeline. If it's not too much trouble, any chance of fixing those things too?

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

P.S just added a minimum support of 9.5 as D8 is EOL.

D9 is also EOL, so we should probably just drop support for that too, no?

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

Then it has to be extremely strict, and exceptionally clear about what the rules are.

So, like @neclimdul, stuff like this is giving me pause:

    // Ensure enums and constants from uninstalled modules can be autoloaded.
    $storage = new AutoloadingStorage($storage, [$name => \Drupal::root() . '/' . $this->extensionPathResolver->getPath($type, $name)]);

What is the justification for allowing this kind of thing?

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

Just now getting into this issue, but I'm pretty spooked by @neclimdul's comment in #78. This has serious implications because, with this change as proposed, config schema can indirectly influence the dependency tree. That's a real danger zone.

If we're going to do that, then I think we need to have some kind of coherent fallback mechanism if a constant or enum cannot be loaded or parsed. It must fail gracefully.

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

That makes sense.

IMHO, we should be looking at this as a snippet of HTML, rather than a full page. What would be a reasonable way of validating that, if we can't use a parser?

I also agree that we don't want to add accidental cross-component dependencies, so this probably needs some refactoring. Maybe a whole new constraint to confirm that something is a valid HTML snippet?

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

I'm not sure that a NULL description is any better then an empty string. Do we think that this part of the change is actually worth it?

I don't feel strongly one way or the other, so I'm assigning this to Wim for input. I suspect he has a clearer reason why NULL would be useful here.

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

Thanks, @narendraR; that makes sense.

However, I agree with @alexpott that NULL is never valid for the site UUID, and so it shouldn't be nullable.

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

Fixed the conflict, will merge when green.

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

Adjusting issue credit.

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

Looks like this got done in πŸ› a Drupal 11 dev line is needed for CROP API. RTBC , so closing this as a duplicate.

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

Gave the MR a quick review and although there a lot of minor changes, I imagine they're mostly there to comply with coding standards, and that's fine with me. The green pipeline is beautiful to see! Merged into 8.x-2.x. Thanks!

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

Self-assigning to start implementing this.

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

phenaproxima β†’ made their first commit to this issue’s fork.

πŸ“Œ | OpenAPI | Add Gitlab CI
πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

phenaproxima β†’ made their first commit to this issue’s fork.

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

I don't have any domain knowledge here but I gave this a code review and there are some things I think could be tightened up, and one outright bug-creating condition that should be fixed before this is merged.

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

Not seeing anything obviously wrong here...let's do it!

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

No objections here.

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

I would be very in favor of some sort of Git trickery here (subtree split?) to facilitate developing recipe suites in a monorepo setup. That's what I'm hoping Starshot will do, and I think it would be useful in other situations too.

Having recipes embedded in other recipes - similar to how modules can contain other modules - would be a hard no from me, as it contradicts the design of the recipe system and breaks the ability to disambiguate them. Let's not add more Drupalisms! :) But it sounds like we're on the same page about that, so I'm just repeating it here for the record.

I'd love to see an example of a recipe suite developed with a subtree split to Packagist. If that's the path Starshot follows (and I'm advocating for that), then I think that could serve as a good example and blaze a trail for other recipe authors.

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

Two very minor things (basically just rewordings), otherwise I think this is OK.

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

I think there are still a few small things that need fixing. Also this is going to need a dedicated change record that we link to, no? Otherwise this is an easy RTBC for me. Looks great!

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

As I was restoring @sime's work in the MR, I realized something...we probably don't want to cache the actual results. That could be really problematic if you've cached a state of "no results", but an error condition is being created in the background.

What we really want to do here is run the status checks less often, I think. So what if we simply cached a flag that said "hey, we ran the status checks at such-and-such a time", and give it a 5-minute TTL? And when it expires from the cache, we run the status checks again.

And, just to cover our asses, we also run the status checks in \Drupal\project_browser\Controller\InstallerController::begin() (clearing the cached last-run-time first).

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

phenaproxima β†’ made their first commit to this issue’s fork.

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

Btw. my main goal so far was to demonstrate for myself (and prove to others) that the browsing expereince was significantly improved with caching. So I'm not really wedded to *how* the caching should work and I was mainly waiting to see how the message improvemnet issue landed because it was calling the validation check.

The more I look at this MR, and the more I examine the test you wrote (thank you!), the less sure I am that we're taking the proper approach here.

For example -- if you previously had no errors, but then a condition changed somewhere and now you have a warning...under the proposed solution (as written in the current MR), you won't know it for at least 30 minutes. That's not going to fly. Or, what if you previously had no errors, but then someone else makes a change that does cause an error? You won't know, because the caching will be covering it up. You'll just hit a nasty surprise when you try to do something with Package Manager.

I don't know if we actually should be caching, exactly. What we should probably be doing is just not running the status checks every time BrowserController is invoked. That's the bug here, which I discovered while working on ✨ Use a route enhancer to move away from having project ID hashes in URLs RTBC . That browse route is used in an extremely awkward way by the frontend.

Here's an idea: rather than running the status checks EVERY time the browse route is hit, why don't we only run it when we're not looking at a particular project? In other words:

    if (array_key_exists('drupalorg_mockapi', $current_sources) && empty($id)) {
      $this->messenger()
        ->addStatus($this->t('Project Browser is currently a prototype, and the projects listed may not be up to date with Drupal.org. For the most updated list of projects, visit <a href=":url">:url</a>', [':url' => 'https://www.drupal.org/project/project_module']))
        ->addStatus($this->t('Your feedback and input are welcome at <a href=":url">:url</a>', [':url' => 'https://www.drupal.org/project/issues/project_browser']));

   // DO THE STATUS CHECKS HERE
    }

This will cause the checks to run a lot less, which should improve performance significantly, even without any caching.

Or, another approach -- what if we created a new endpoint that did the status checks, and the frontend could call out to it whenever it felt like that was necessary (i.e., just before clicking the "Install" button)?

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

I think this is looking good but there are a couple of sketchy things that need cleaning up. Otherwise, though, this looks like it does what's advertised.

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

phenaproxima β†’ created an issue.

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

To do this, we also need to bump the composer/installers requirement to ^2.3.

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

Okay, I looked into it and this is yet another wall of assumptions Project Browser makes, where everything is a module and the machine name is the globally unique identifier. We need to change that assumption, but it's going to take some work...

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

Okay, I don't get it...looking at BrowserController, the $module_name param is never used in any meaningful way. The controller's output is always the same, regardless of what $module_name is.

I'm not sure this is actually used anywhere. Yet the Svelte code does reference it, for some reason.

I'll look into this.

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

this method is changing to `createIfNotExists`

Yup, that's correct. In fact, let's just directly deprecate ensure_exists and replace it with createIfNotExists here.

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

A one-line copypasta bug fix that makes the recipe application process smoother and reduces unexpected exceptions? SOLD! Heartily, I RTBC thee.

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

Awesome detective work, @bsharpe! I think we'll need @alexpott to weigh in here.

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

✨ Create a centralized, non-volatile repository of all projects known to all sources Needs review is assigning every project a UUID, so maybe we should block this issue on that one and make it so that we use the UUID to identify the project? :)

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

You're right that the pipeline will show the deprecations, but that's not what we want. We need explicit automated test coverage of the deprecation, and we don't want the pipeline itself to show any, because that indicates that we're still using deprecated code in core.

An example of how to explicitly test for a deprecation might be seen in \Drupal\Tests\system\Functional\Module\DeprecatedTemplateTest::testDeprecatedTemplate()

(Basically -- your test method must be annotated with @group legacy, and you need to call $this->expectDeprecation(...) before you actually do the thing that will trigger the deprecation, which in this case is creating an instance of ensure_exists or simple_config_update.)

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

Thanks for working on this. We'll still need some light test coverage, per #9, to prove that the deprecated plugin IDs are still usable, but trigger a deprecation error.

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

Tagging for an issue summary update; the answers I'm asking for in #17 should be reproduced there.

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

Also, I need clarification on something: the issue summary says we're running the status check on every page, but I see no evidence of that? It's only run, as far as I can tell, by BrowserController, which is only invoked when you...load the project browser?

Is it possible there is something else causing the performance issues? Not saying it's necessarily bad to cache the results, because they certainly are expensive to compute, but I'm wondering how serious of a problem this actually is, and whether we're actually going to realize big performance gains across multiple routes by doing this. By no means is Project Browser unusable, even with the expensive status checks.

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

Tagging this for further profiling before commit, so we can be sure we're actually having a positive effect on performance by adding this cache layer.

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

No, no, we should be removing that line entirely. Testing modules don't need to declare core compatibility.

Production build 0.69.0 2024