@heddn

heddn → made their first commit to this issue’s fork.

On second thought, we'll also want to post a CR for this. I'll do that after I get a review.

It runs on the testbot, but at a huge cost, 36 minutes: https://git.drupalcode.org/issue/drupal-1565704/-/pipelines/397114/test_...

Update tests added and are green now. IS seems up to date. Ready for review.

Let's fix this then.

As I expected, I tracked down over in https://www.php.net/manual/en/ini.core.php#ini.sect.file-uploads that we can't set that value at run-time.

This can't be easily tested with the current approach. Perhaps the testbot CI can handle things, but running the test locally, (5000 max input var is my php's default) and the test never runs to completion. I tried to set the value lower via an ini_set in the test's setup method. But that still left the test form still using 5000 from max_input_vars.

Instead I manually tested things with the test by artificually setting my max_input_vars to 5. That let things run to completion in a very happy way. Moving this back to NW because we either need to be OK with manual testing (and get rid of the tests) or figure out a way to runtime set the max_input_vars to a lower value.

LGTM. All concerns addressed.

heddn → made their first commit to this issue’s fork.

There's tests. All feedback is addressed. LGTM.

re: slot count:

From #1199866-84: Add an in-memory LRU cache → , the thought was that this should be a much higher number then 300. So it is nice to see a number of 1000. Do we know if smaller hosting, e.g. an entry level pantheon plan will hold 1000 entity slots for 5-6 fields on a node or paragraph?

Since Benji RTBCed this, I'm going to assume his suggestions/comments on the MR can be resolved at this point. That is now done. Otherwise, +1 on RTBC.

Added test coverage. This was RTBC, so hopefully someone can simply re-RTBC after reviewing the tests.

heddn → made their first commit to this issue’s fork.

Made a few minor fixes given the last few asks. This looks pretty good at this point. Given all I did was switch a comment to {@inheritdoc}, I think I can still mark this RTBC.

heddn → made their first commit to this issue’s fork.

I've gone through and responded to most of the feedback mentioned in the IS. The requests in #80 & #82 are not super clear what is being asked given that the EnvironmentMemory object didn't materialize. Can we cross them off as well?

This is postponed on 🐛 SqlBase::prepareQuery() should be called also on count Needs work .

My most recent visit to this issue is from ✨ SQL source plugins: allow defining conditions and join in migration yml Needs work , which is the nearly the number #1 asked for feature for migrate.module right now. I'd hate to block real progress on a lot of nice to haves.

I did a rebase of the MR. I'm not sure what are the actionable next steps here. From reading #46, there's a whole rework desired. What is the MVP?

heddn → made their first commit to this issue’s fork.

heddn → changed the visibility of the branch 3069776-sql-source to hidden.

heddn → changed the visibility of the branch 3069776-sql-source-plugins to hidden.

heddn → changed the visibility of the branch 11.x to hidden.

This is pretty close. Can we fix the phpcs, etc issues and add a test case? That's all that is missing from landing this.

In general, I like the approach discussed in #74. It might not solve all the problems, but it does solve a single problem and should help with some people's issues. Let's roll it into an MR and add tests.

Nice improvement to an existing migration to make this more flexible. There's tests that are passing. LGTM.

Thanks for the contribution. I've added a test that will help us catch this in the future.

heddn → made their first commit to this issue’s fork.

This is no longer a concern as it got solved in the D10 compat issue.

Replied in MR. Thanks for your kind review.

heddn → created an issue.

heddn → changed the visibility of the branch project-update-bot-only to hidden.

heddn → changed the visibility of the branch 3435465-automated-drupal-11 to hidden.

heddn → changed the visibility of the branch 3435465-d11_ready to hidden.

Can you provide some examples/links where you are contributing already to the module? That would definately improve your chances of getting maintainership access.

heddn → made their first commit to this issue’s fork.

Closing as duplicate of 📌 t() calls should be avoided in classes. Active .

Thanks #7 for your input. I typically would agree with you. But since this and 📌 \Drupal calls should be avoided in classes, use dependency injection instead Needs review are dealing with 2 different phpcs things I wonder if we should just re-title/re-purpose to fix all phpcs issues in one shot. Instead of having to review several distinct issues. With that mind, re-titling and closing the other as duplicate to this.

NW to address all PHPCS.

heddn → made their first commit to this issue’s fork.

heddn → made their first commit to this issue’s fork.

Let's merge and see what the users of the community say.

heddn → made their first commit to this issue’s fork.

Will need to be rerolled into an MR at this point so tests can execute.

Will need to be rerolled into an MR at this point so tests can execute.

Thanks for the contributions.

heddn → made their first commit to this issue’s fork.

Let's move this over the the 2.x branch. Will need to be rerolled into an MR at this point so tests can execute.

Let's move this over the the 2.x branch. Will need to be rerolled into an MR at this point so tests can execute.

With the EOL of D7 past, let's close out old stuffs. If this is still an issue in the more modern version, feel free to re-open and document steps to reproduce.

With the EOL of D7 past, let's close out old stuffs. If this is still an issue in the more modern version, feel free to re-open and document steps to reproduce.

With the EOL of D7 past, let's close out old stuffs. If this is still an issue in the more modern version, feel free to re-open and document steps to reproduce.

With the EOL of D7 past, let's close out old stuffs. If this is still an issue in the more modern version, feel free to re-open and document steps to reproduce.

With the EOL of D7 past, let's close out old stuffs. If this is still an issue in the more modern version, feel free to re-open and document steps to reproduce.

With the EOL of D7 past, let's close out old stuffs. If this is still an issue in the more modern version, feel free to re-open and document steps to reproduce.

Thxs

heddn → made their first commit to this issue’s fork.

This is for the D7 version of the module and doesn't seem to apply to the more modern version. Please re-open if it is still an issue on that version.

My changes to the tests are so minimal, I'm going to say this is good to go. LGTM

Now that 💬 Revert requirement that IDs not include spaces Needs work landed, I think this can be closed as outdated. If that isn't the case, please re-open.

Thanks for your contributions.

heddn → created an issue.

Thanks everyone.

Open an MR with tests?

Fixed failing tests by converting to a data provider and a kernel test. There isn't a 100% replacement as many of the previous tests were simple testing of setter/getters. Needs a review.

The changes to drupal core were actually added in 10.1. You don't need to do anything except uninstall advagg.

I would mark this as RTBC except I made some minor changes to the tests to get them to green. Anyone else able to make that call?

Unit tests are now passing green on MR 15.

heddn → made their first commit to this issue’s fork.

RTBC++

re #280: my site's config was already using sub_handler and sub_handler_settings with a recent version of the patch. I'm about to switch to the contrib module with the core code in 10.4.0, but wanted to provide this insight.

Can we roll that patch into an MR? Tests no longer get triggered on patches.

There's a lot of re-work needed to make this work w/ CK5 and D11. https://www.drupal.org/node/3291493 → deprecated the image dialog forms. I'm going to leave this at NW for now. But I started some of the rebase work in https://git.drupalcode.org/project/drupal/-/merge_requests/10767. It is a pure rebase. From here on, we'll need to figure out how to make this work with CK5. I'm not taking that on at the moment.

Can you open a follow-up to remove the old state entry? We should get rid of it.

Thanks for your contributions.

CR added

With force pushes and a series of confusing commits, I'm not sure what has happened since #12 when I last RTBCed this thing. It looks like my changes even disappeared. And we've regressed from green passing tests to failures. This is not RTBC ready.

I'm pretty sure this needs a rebase.

Errors fixed in readme. Thanks.

heddn → made their first commit to this issue’s fork.

Let's add test coverage so regressions like this can't happen.

We have Guzzle available to us so the curl isn't really needed.

Also, I'm not sure I like the idea of having remote resources download-able by default with this module. That opens up the attack surface quite a bit. Being able to do any prep work for a resource could/should happen before the migration happens. Say via a shell script that curl/downloads the resources locally first. Convince me why this is strictly necessary.

Thanks for the graphics.

Thanks for the contributions. This makes a lot of sense.

heddn → made their first commit to this issue’s fork.

heddn → made their first commit to this issue’s fork.

Instead of defaulting to user=1, let's make this a configurable for _which_ user we want to execute as. Similar to how drush used to work .i.e. drush mim --uid=33 my_migration. My logic being that user=1 is no longer always a full admin user since https://www.drupal.org/node/2910500 →

Thanks for your contributions.

Can we add a test for this since we've already had a regression?

re #35: A small side note, stating that tests found regressions is kinda of saying that the tests are doing their job. No one likes random failures but if a test finds issues, it isn't necessarily the tests fault. I sense some resistance to the performance testing addition. We should be embracing these things.

Posted an question, but otherwise, I think this is pretty close.