Michigan, USA
Account created on 29 July 2014, over 9 years ago
#

Merge Requests

More

Recent comments

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Lol. We could, except this project has never had a stable release, so there's never been anything to cover. We went from 7.x-dev to 8.x-1-rc2, and we are back at alpha now. πŸ˜‚πŸ˜‚ I never got the 1.x branch to a point I was comfortable to call stable. Hopefully soon, with all the recent activity.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA
πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

No longer postponed, as πŸ“Œ Remove usages of MigrateSkipProcessException from core process plugins Fixed has been merged

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

I would argue it is exactly what exceptions are made for: to break out from the ordinary code flow.

But we don't want to break out of the normal code flow here. Skipping rows and finalizing pipelines doesn't happen because of an exceptional condition, bad data, or an error. These are operations that are done by process functions in their normal workflow, and expected and supported behaviors. They are operations on an object, and we have the object available, so it should be able to know its status.

SubProcess shows the need for manually propagate instead of the exception automatically doing so.

I'm not sure precisely what you mean here. I'll assume that you are referring to the fact that the MR has to set the row to skip when it encounters a condition where it wants the row to skip. This is first off only being done to preserve buggy behavior that was partially exposed by this issue. πŸ› When sub_process encounters a row skip, it should skip its internal row, and not bubble up to the outer row RTBC is attempting to fix that bug. It's a prime example of the issues caused by using exceptions to signal things in the normal flow of execution. Some called function deep inside of subprocess throws a Skip Row exception, and subprocess didn't think to handle it, and it bubbles up and breaks the whole outer row. This makes it more difficult to code those kind of bugs.

Look at the number of checks for skipping before and after. What use cases are made possible by adding this complexity?

This code is in a transition state, but the ultimate goal here is to remove the usage of MigrateSkipRowException and MigrateException (the latter of which does the same thing as MigrateSkipRowException, but allows you to set an arbitrary row status while bypassing the row) in process plugins, and replace them with a single set of api calls on the row object itself. This makes logical sense to me, as the row object should know and report its status. As far as the number of skip checks, with the exception of sub_process, these are 1:1 with the existing try-catch blocks which are going away. and πŸ› When sub_process encounters a row skip, it should skip its internal row, and not bubble up to the outer row RTBC currently adds a try-catch block there too, which will be replaced by a skip check either here or there depending on which issue is committed first. The endgame code will be able to replace multiple catch blocks with a single API check, so we are reducing complexity.

What problems are being solved here?

See above comments on reducing complexity and preventing unintended bugs

Also, I am not sure why is it called "getSkip". Let's look at events: the method is called isPropagationStopped so the analogue would be isProcessingSkipped.

Alwasy happy to debate naming conventions, the PR is a WIP, but initially it as a simple getter and was named as such, but something like isRowSkipped would work too. I'm going to see what it all looks like once the migrateException stuff is included and see what makes the most sense.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Updated the MR, fixed conflicts, and added in the change to the validator in the patch in #123 Still need to address @longwave's comments in #119

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

mikelutz β†’ changed the visibility of the branch 2329253-allow-the-changeditem to hidden.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Assigning to me for more tests and features, and requested changes.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Hmm, I'm not sure this will be something we can directly support in the module if the pelcro JS is firing a subscription create event prior to payment being completed. We would have to see if there is any way for us to tell inside that event that the payment has not completed, and ensure that that event or another one is fired after payment is actually completed. Based on the documentation here: https://docs.pelcro.com/v1.67/docs/js-sdk#events. I'm not sure that either of those is the case. IF pelcro is creating the subscription prior to payment being completed (or at least firing the event) we may have trouble with this.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

It doesn't appear to need a rebase. That issue was merged on Jan 12, and I picked this back up and rebased on Jan 13. It's still NW because the refactor needs more tests, and I'm playing with adding the ability to set the row map status on skip, which would let us deprecate using MigrateException inside a process plugin to skip a row and mark it failed. Hoping to get to it this week.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Dang. I had a whole thing typed up here, but I must not have hit submit. Here goes trying to remember what I was going to post...

This is a bug, NW for tests, but I'm not certain if this is the right approach here.

So the core EntityConfigBase expects the entity id key (generally id for configs) to contain the whole id. for a field storage entity, that would look like "node.field_text' we have 12 plugins that extend EntityConfigBase for various reasons. The three addressed in the MR are the only ones affected by this bug, as they override the getIds() method to change the destination plugin's ids and map table storage such that the individual parts of the config id are separate properties in the migration (i.e entity_type: node, field_name: field_text)

And as mentioned above, and fixed in this patch is the issue that the base getEntity method (from \Drupal\migrate\Plugin\migrate\destination\Entity) can't handle that, it tries to load an entity using only the first element of the ids array.

So there are two interesting things to note here. The first is EntityBaseFieldOverride which needs to do a similar thing but does it in a different way. Instead of overriding getIds, it overrides getEntityId instead. I got a bit into the weeds with trying to understand it, but it seems like this works for base_field_override, and might have worked for field instances because of the FieldConfigBase id() method, but it won't work in general.

The main thing that give me pause is this bit of EntityConfigBase's import method:

    if (count($ids) > 1) {
      // Ids is keyed by the key name so grab the keys.
      $id_keys = array_keys($ids);
      if (!$row->getDestinationProperty($id_key)) {
        // Set the ID into the destination in for form "val1.val2.val3".
        $row->setDestinationProperty($id_key, $this->generateId($row, $id_keys));
      }
    }

So, EntityConfigBase is smart enough to know that if a subclass has overridden the getIds() method, then it needs to implode the non langcode ids with a '.' separator and load it into the 'id' property so the config can be created with the right id, but it doesn't do the same with the old_destination_id_values before calling getEntity(). So my gut says we should fix this in EntityConfigBase, either by manuipulating old_destination_id_values before calling getEntity(), or more likely by overriding getEntity and putting the logic there (That way subclasses could still override getEntity with the original old_destination_id_values if they wanted to.

NW to refactor this into EntityConfigBase for any subclass that overrides getIds() and for tests.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Made some adjustments that broke some tests, and I'm still debating if they are worth keeping at all. Currently, when SourcePluginBase runs all the prepare_row hooks, if a hook return false, the remaining hooks still run, and the row is skipped, while if a hook throws a skip_row exception, the remaining hooks are skipped and we go straight to the catch block after the hook invocations. The original version of this patch left it so that calling $row->skip in a prepare row hook still let all the remaining hooks run before deciding to skip the row. While this difference in behavior from the exception is fine (We don't rely on the behavior in core, and it's okay for the replacement of a deprecated api to work slightly differently) for performance reasons, it seems like it would be nice to stop hook processing once a skip is flagged. The latest commits do that, but broke a few tests that will be a little tricky to rework. I think ultimately we should keep the efficient behavior and eventually deprecate the ability for prepare_row hooks to return FALSE as a signal to skip a row, requiring them to use $row->skip instead, but I don't have time to fix all the tests today.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

✨ Allow process plugins to stop further processing on a pipeline RTBC has now been merged and this issue is no longer postponed.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

MR updated and passing. Ready for review.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Everybody saw it, Longwave said I could change the interface!

Seriously, that is my preference, I've just gotten pushback in the past when following the BC promise to the letter when there is an alternative that goes above and beyond, hence the new interface. It does seem like lately we moving away from going above and beyond the BC promise when doing so involves adding extra unnecessary code and interfaces, so I'm all for it.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

This is a bug, The explode plugin should accept any value that PHP's 'explode' function will accept, which is any string not equal to the empty string ("")

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Cross post with cilefen. I didn't mean to undo his status change, though I'm still unsure how πŸ› Regression from #2521800: using machine name element for ListStringItem breaks with existing data Fixed would affect the actual settings form.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

The issue with the fields is related to πŸ› Regression from #2521800: using machine name element for ListStringItem breaks with existing data Fixed But I do not immediately see why that would prevent saving the settings form itself. That issue is specifically related to list fields on entities, it shouldn't affect a config form, and I see nothing in core behind the scenes that would trigger any validation around the fields when saving just the settings form.

Un-checking the "Contact settings" prevents a new user from trying to register
while the test system is online. Does anyone happen to know the area in the
Drupal database that the "Contact settings" can be set, which is likely a simple
boolean value?

Contact settings would be stored in the 'user_default_enabled' key in the contact.settings config file, though new user registration would be the 'register' key in the user.settings config file, where a value of 'admin_only' would prevent registration. Both are simple configurations that could be exported at /admin/config/development/configuration/single/export and copied into /admin/config/development/configuration/single/import with a modification and saved

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

CR looks good, new test looks good. The "fix" is very straightforward, most of the discussion on this issue is around whether to define this as a bug, and I'm confident that it is. The CR provides options for those that wish to replicate the buggy behavior. I'm marking this RTBC.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

This needs to have the merge request updated to 11.x, or a new MR created against 11.x

My only concern with the approach is that the prepare row method and hooks can also write messages to the table, and if they aren't explicitly skipping the row by returning false then any messages created by the row preparing will start accumulating. Maybe we should check whether any messages exist for the row before we restore instead of just checking if the row will be skipped due to track changes/highwater/needs update etc.

I was also a little worried about performance, I ran some tests on a 40k row migration without the map table joined. The migration was completed, so no rows were actually processed. I inserted a message for each row and ran a few times with and without the patch. I was seeing ~15seconds or 6% additional runtime on average with the extra database trips per row. It's not insignificant, but it could be worse, it extrapolated to an extra 6ish minutes on a million row run that would have taken an hour and a half already, so maybe it's worth providing a way to opt out? I'd approve the MR without an opt-out, but I'll let others weigh in.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Would it be easier to just change it to say 'Drupal version 8 or later' if we don't explicitly detect 6 or 7? The migrate UI is only designed for 6 or 7, so it doesn't much matter that we accurately find out the specific version, only that we accurately determine if it's 6 or 7, or something we don't handle.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Setting to NW to add documentation to all plugins we are creating or changing, and for a CR.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Chatted with @alexpott in Slack about this, and I feel better. As mentioned, historically we were testing with YamlPecl, and it is important that we go back to that, to avoid breaking things with the extension, and it seems we have enough compatibility tests to be comfortable with doing so.

I am curious though, (and I haven’t had enough coffee this morning to wade through the spec) whether that format is valid YAML or not according to the YAML spec. if it is, then maybe there should be an issue against symfonyYaml to allow it.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Saw this in #contribute and was curious. I haven't been involved in the YamlPecl stuff to date, so I may be wrong here, but I'm wondering if someone can tell me if I am off here.

From looking at to \Drupal\Component\Serialization\Yaml we always encode via Symphony Yaml, but we decode with YamlPecl if it is available, and fall back to SymphonyYaml if it isn't.

do not:
      do this for the love of Foo Bar!

is valid yaml in yamlPecl but not symfonyYaml.

If we change the test for invalid yaml to something that is invalid on both, and then add PeclYaml to the test containers, does that put us in a position where we could commit some config with the above format, have it pass all tests (as we are using PeclYaml), and then be broken a system that is relying on SymfonyYaml? Do we lint yaml files to the symfony standard somewhere to make sure we are not potentially committing something that Symfony would find invalid? Are there other concerns that if we add YamlPecl to the test containers that we are then not testing anything with the SymfonyYaml fallback? Given than SymfonyYaml and YamlPecl have different opinions on the above format, I wonder what other differences might be lurking that we won't catch later if we take SymfonyYaml out of tests.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Alright, I think we can close MR5299 and go with MR5462 for this issue then. Leaving as NR from NRQI.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

I don't fully agree. If we were using the normal drupal logger, then yes, we can mock that in memory and it's fine. The idmap plugins are their own system, and the Sql Idmap is used 99.9% of the time. I feel like what you want to test here is what message actually makes it into the database, and I think that should include the system that is responsible for storing it in the database. I also don't think the performance costs of the the SQL plugin are significant compared to the costs of running a migration in the test, I feel the additional coverage of the system used to store the messages far outweighs the extra few clock cycles to include it.

That said, I also don't mind keeping tests very specific and isolated. If the goal here is to test the message that gets passed to MigrateIdMapInterface::saveMessage() given a specific process exception thrown in a specific place, I fully support that. We have \Drupal\tests\Migrate\Unit\MigrateSqlIdMapTest::testMessageSave() to test that the message passed to saveMessage() is what makes it to the database.

So if the goal is to test the argument passed to saveMessage is as expected given a process exception, lets do that directly using prophecies, rather than making. a new test module. I created a new MR using that approach. I prefer the prophecies to adding test modules when possible. While I admit this test comes close to the borderline where a complex prophecy setup might make me prefer a test module, it does not cross it for me. The benefit of having the entire test in one class makes this preferable, I believe.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

@xjm This is only a helper function for PHPUnit tests. The regular plugin manager will properly sort migrations via a dependency graph when requested. I don't believe it can cause data integrity issues on sites.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

@berdir IF you are interested in doing this, I think I flagged the non-obvious fixes that need maintainer eyes.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Nope, didn't realize that was still the default branch. I'm working on it. :-)

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

mikelutz β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

With core migrate, we get a lot of migration plugins that we never want to execute, and which we only want to use as templates.

I just want to note, these aren't meant to be templates, these are actual migrations, intended to be run to upgrade a site, and the yml plugins provided by all of core and contrib are built with the intention that they be run. Many people use them as templates to build their own migration plugins or configs, but we can't just assume that they are to be treated only as templates.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

I don't think this is the right approach here. Currently the redirect to the base_url isn't happening in the logout hook, it's happening in the event subscriber that checks for session expiry.

  /**
   * Logs out user if not SAML authenticated and local logins are disabled.
   *
   * @param \Symfony\Component\HttpKernel\Event\RequestEvent $event
   *   The subscribed event.
   */
  public function checkAuthStatus(RequestEvent $event) {
    if ($this->account->isAnonymous()) {
      return;
    }

    if (!$this->simplesaml->isActivated()) {
      return;
    }

    if ($this->simplesaml->isAuthenticated()) {
      return;
    }

    if ($this->config->get('allow.default_login')) {

      $allowed_uids = explode(',', $this->config->get('allow.default_login_users'));
      if (in_array($this->account->id(), $allowed_uids)) {
        return;
      }

      $allowed_roles = $this->config->get('allow.default_login_roles');
      if (array_intersect($this->account->getRoles(), $allowed_roles)) {
        return;
      }
    }

    if ($this->config->get('debug')) {
      $this->logger->debug('User %name not authorized to log in using local account.', ['%name' => $this->account->getAccountName()]);
    }
    user_logout();

    $response = new RedirectResponse('/', RedirectResponse::HTTP_FOUND);
    $event->setResponse($response);
    $event->stopPropagation();

  }

Since the logout hook no longer redirects when isAuthenticated() returns false, we are falling back to the redirect response to the event here, redirecting back to the homepage. It seems like we should leave the logout hook alone, and handle redirecting on session expiry here in the event. I can also see a need for this to be configurable, or at the very least a bit smarter. If feels like in most cases the user should be redirected to the login page, and then returned to the current url after logging in, though I could see use cases where a site might want to just let them view the page as anonymous without being redirected. I have a need to solve this this week, so I'll open a MR with my solution soon.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

I verified the new MR matches the patch in #4. Tests pass and the test-only job fails as expected. Restoring RTBC.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Compared the MR generated patch to the original patch, no changes other than a couple line numbers. Restoring RTBC pending passing tests.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Yes, we should probably throw an exception in the first case. The reason we don't is because of stubbing, when we run migrations with null source values, we don't want the subprocess to explode when it gets null instead of an empty array. We could make this more explicit, though, return an empty array on null only, and document it all better.

The second case doesn't make any sense to use subprocess on. Sounds like you want to treat an array of scalars as if it's a single subrow to process, but that's not what subprocess does, and you wouldn't need subprocess, you could just set property/0/subfield:property/subfield directly.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Yeah, I don't think #5 was about our code having links to third party websites. It was about modifications in assets/vendor, which is vendor code that we directly copy into our repo. Those files should not diverge from the vendor versions.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Merging into 2.0.x branch. Thanks!

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

I'm going with Dan's approach here, I think it's cleaner. Committed and pushed to the 2.0.x branch. Thanks!

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

2.0.x branch is open now, and updated for D10. I'll probably drop an alpha with just the d10 fixes, but if you still want to work on this, we can include it in a beta.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Merged to 2.0.x branch. Thanks!

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA
πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA
πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

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

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA
πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

mikelutz β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

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

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Committed and pushed to 2.0.x branch. Thanks!

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA
πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Committed and pushed to 2.0.x branch. Thanks!

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

I can kick out a D10 dev release. I built this years ago for a specific project, and don't actively use it anymore, and with only 30 some installs, I confess I've neglected it.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA
πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Committed and pushed to new 2.0.x branch

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Unrelated test failure.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Ignore this, I was looking at a patched copy of this module and didn't realize it.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

My mistake, those aren't the primary full upgrade tests, those are the full upgrade tests for the forum module. Those failures come from not enabling the language module despite the language migration being required for some of the migrations being run. The test itself will need a few updates to pass with that module enabled, which I will try to work through.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Thanks, that snuck in there from an earlier version of the test I wrote, and didn't get removed when I refactored. You cleaned it up before I got back to fix it. Confirming that the fix was a trivial removal of a unneeded use statement, and you should still be able to RTBC.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Sigh.. it's never good when it's down to just the full upgrade tests failing.. mean something isn't being tested at the kernel level that should be..

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

All that to say, I think we can commit this without the assertNotEmpty check. The point of that was to make it harder to write tests with mistakes, but digging into it today, they fact that we can call createInstances() with invalid plugin ids and have it silently fail has caused mistakes in runtime code and migrations, so we need to fix those, and in doing so will make this check redundant anyway.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

Yeah, I'm still working through the ramifications of all this, trying to see if we can fix it, or if we are going to need to add in a BC layer.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

#7 is right in line with my hopes and dreams. If we deprecate in 11 and remove in 12, I don't feel any obligation to proved a contrib version of migrate Drupal. Anybody in the community is welcome to put one together, but it would not be a responsibility that we as migration system maintainers would take on directly.

This is also inline with the maintainer consensus at the last maintainer video, though I'll let everybody else weigh in, now that it's written out like this.

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

πŸ“Œ MigrationPluginManager::createInstances claims to throw a PluginException on invalid ids, but does not. Needs review exposed even more issues... because expandPluginIds silently filters out non-existent migration ids, and we run the migration requirements through expandPluginIds before checking that the rows are processed, we actually aren't checking that required migrations exist when checking requirements. We only check that if they do exist that they have been fully processed...

πŸ‡ΊπŸ‡ΈUnited States mikelutz Michigan, USA

So this seems to open up a whole can of worms before and after πŸ“Œ $migration_dependencies has inconsistent structure Fixed . Some of these tests just need an adjustment to expecting the exception instead of checking that createInstances returns an empty array. But more importantly, this shows migrations like the d6 block which has

migration_dependencies:
  required:
    - menu
    - d6_custom_block
    - d6_user_role

There is no 'menu' migration. There are d6_menu and d7_menu migrations. But the plugin manager pumps the migration_requirements through expandPluginIds before building the requirements array, and thus stripping out any non-existent migrations. So migrations requirements check that migrations that exist have run to completion, but never checks that the migrations listed in the requirements actually exist. If they don't exist, they are just skipped over...

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