Hello all, it’s time for the weekly migration subsystem meeting. The meeting will take place in slack in various threads
This meeting:
➤ Is for core migrate maintainers and developers and anybody else in the community with an interest in migrations
➤ Usually happens every Thursday and alternates between 1400 and 2100 UTC.
➤ Is done on the #migration channel in Drupal Slack (see www.drupal.org/slack for information).
➤ Happens in threads, which you can follow to be notified of new replies even if you don’t comment in the thread. You may also join the meeting later and participate asynchronously!
➤ Has a public agenda anyone can add to. See the parent issue for an idea of the typical agenda.
➤*Transcript will be exported and posted* to the agenda issue. For anonymous comments, start with a :bust_in_silhouette: emoji. To take a comment or thread off the record, start with a :no_entry_sign: emoji.

Core migration issues

Next video meeting 2022-07-21 2100Z

The hope is that most or all of the maintainers will attend. We will try to focus on longer-term goals than in the weekly meeting.

0️⃣ Who is here today?

mikelutz (he/him) Partially here on my phone.
benjifisher I am in Asheville, NC in the US. This will be my second in-person Drupal Camp since the pandemic started.
darch I’m here, currently in Nicaragua (though actually based out of Arizona)
benjifisher Hello, and welcome to the meeting. Thanks for joining!
quietone Hi
Nick Dickinson-Wilde Nick, he/him, coming in from the North (Canada)

1️⃣ What should we talk about today? Suggest topics here and I will add threads. I will also check for comments on the issue for today's meeting.

mikelutz (he/him) #3285637: 'Get' Process plugin should handle multiple

2️⃣ Action items. To be added later.

benjifisher Review #3285637: 'Get' Process plugin should handle multiple. @benjifisher, @danflanagan8
benjifisher Review #2953111: Only migrate role permissions that exist on the destination. @benjifisher
benjifisher Test #3063856: Add ability to view migrate_message table data. @darch

3️⃣ Statistics

benjifisher Fixed since last week's meeting: 1 (not counting the issue for the meeting).
benjifisher RTBC: 8, 1 of which is Major.
benjifisher NR: 16, including 1 Critical, 2 Major, and 4 of which have not been updated in more than 2 months.
benjifisher Google sheet for recording stats: https://docs.google.com/spreadsheets/d/1o0Rjlc1vnnLP5bM5P-SMMyGzqn7258hi... (not yet updated for this week)

4️⃣ Comment in this thread if you are looking for ways to help. Give us some idea of what you would like to do: documentation, code review, testing, project management, ...

darch I am willing to help but not sure what would be most useful. I am a drupal dev as my day job so probably can help on issues and documentation, testing, etc. Don’t really have a preference but do want to start contributing back in the community
benjifisher I have not looked at the issue in 8️⃣ in a while. Would you like to test that?
darch sure!
benjifisher Sometimes I spend a long time doing code review, and I have to remind myself about the "T" in RTBC.

5️⃣ Previous minutes.

benjifisher NR:2022-06-16: #3293617: [meeting] Migrate Meeting 2022-06-30 1400Z

6️⃣ Announcements

7️⃣ Only migrate role permissions that exist on the destination

benjifisher #2953111: Only migrate role permissions that exist on the destination
benjifisher This issue has been through a round of changes in the last week. I have to review the latest patch.
benjifisher I think it is close.
benjifisher Currently NR, Critical.
quietone I need to comment about the fail test. I don't think one is needed.

8️⃣ Expose full set of debugging data in migrate_message table (filterable/searchable)

benjifisher #3063856: Add ability to view migrate_message table data
benjifisher This issue (Major, NR) is next on my list after 7️⃣.

9️⃣ 'Get' Process plugin should handle multiple

benjifisher #3285637: 'Get' Process plugin should handle multiple
benjifisher @mikelutz (he/him): we discussed this issue a few weeks ago. It looks and walks like a bugfix, and so far no one has come up with any way it would break anything. Judging by the comments on the issue, @danflanagan8 has tried harder than most.
At this point, are you looking for code review? (edited)
mikelutz (he/him) Dan commented on favor, I believe. We also found another ticket that ended up being this bug. Looking for an RTBC from someone.
danflanagan8 I need to review the tests more closely. In reviewing this one it has become clear to me that I still struggle with handle_multiple and multiple() .
danflanagan8 But, yes, I am in favor at this point. Just haven’t done quite enough review to RTBC it.
benjifisher I am having a look now, but I will not go too deep if you are also reviewing it.
danflanagan8 I won’t be able to do a through review now, for sure
danflanagan8 Work is making me work and it’s really annoying!
benjifisher I think there is too much copy/paste in the code comments in the test. Since @mikelutz (he/him) is on his phone right now, I will add a comment to the issue with that.
mikelutz (he/him) I fixed those. Good catch.

1️⃣0️⃣ Testing system tries to save the wrong stream when using Guzzle

benjifisher #3292980: Testing system should explain why Guzzle responses can be unreadable
benjifisher This is a follow-up to the critical bug we discussed last week. It turns out that that was a bug in the testing system. The bug was exposed by one f the migration tests.
I confirmed the explanation with an automated test. This issue has that test and adds a code comment. I am looking for a code review.
mikelutz (he/him) The code comment is fine, the test you added probably should not get committed. You are just testing guzzle.
benjifisher That is true. If we cannot find any documentation saying that Guzzle is supposed to work like that, then I would like to keep the test. If it is not documented, then it is subject to change. If it changes, then we may have to change our testing code.
mikelutz (he/him) I can't imagine a framework manager would allow a test of a vendor library like that. I also am not sure what documentation you are looking for. It's documented that sink is a stream. This is just how streams work, I don't know what precisely they would add to the documentation.
benjifisher The documentation I am looking for is the effect of the 'sink' option on the return value. The sink is a stream, but where does it say that it will be returned as the body of the response.
If you are still not convinced, then I will upload a patch with just the comment. I will copy some of this discussion to the issue, unless you do so first.
benjifisher Our maybe some day we replace Guzzle with Gazzle, which is mostly interchangeable but handles sinks differently. The test fails, and we know we have to update our code. Maybe the update is just removing the unneeded test for isReadable().
mikelutz (he/him)
/**
 * sink: (resource|string|StreamInterface) Where the data of the
 * response is written to. Defaults to a PHP temp stream. Providing a
 * string will write data to a file by the given name.
 */
const SINK = 'sink';
mikelutz (he/him) It always dumps the download into a stream and returns that when you call getBody().  By default that stream is php://temp which is opened r+.  The download is drained into the sink, the sink seek index is reset to 0 and that’s always what you get back. (edited)
mikelutz (he/him) We are overriding that php://temp with a file stream that WE CHOSE to open w instead of r+
mikelutz (he/him) The only way to get the actual download stream is to set
/**
* stream: Set to true to attempt to stream a response rather than
* download it all up-front.
*/
const STREAM = 'stream';
mikelutz (he/him) If you really wanted to do this right, we should open the file r+ instead of w.. then the response would act the same.
mikelutz (he/him)
list($stream, $headers) = $this->checkDecode($options, $headers, $stream);
$stream = Psr7\stream_for($stream);
$sink = $stream;

if (strcasecmp('HEAD', $request->getMethod())) {
    $sink = $this->createSink($stream, $options);
}

$response = new Psr7\Response($status, $headers, $sink, $ver, $reason);

if (isset($options['on_headers'])) {
    try {
        $options['on_headers']($response);
    } catch (\Exception $e) {
        $msg = 'An error was encountered during the on_headers event';
        $ex = new RequestException($msg, $request, $response, $e);
        return \GuzzleHttp\Promise\rejection_for($ex);
    }
}

// Do not drain when the request is a HEAD request because they have
// no body.
if ($sink !== $stream) {
    $this->drain(
        $stream,
        $sink,
        $response->getHeaderLine('Content-Length')
    );
}

There’s the meat from StreamHandler::createResponse(), but the good stuff is in createSink()

mikelutz (he/him)
private function createSink(StreamInterface $stream, array $options)
{
    if (!empty($options['stream'])) {
        return $stream;
    }

    $sink = isset($options['sink'])
        ? $options['sink']
        : fopen('php://temp', 'r+');

    return is_string($sink)
        ? new Psr7\LazyOpenStream($sink, 'w+')
        : Psr7\stream_for($sink);
}
mikelutz (he/him) It opens a stream and drains the download into it and sets that as the response either way. It’s not unreadable because we override the sink, it’s unreadable because we overrode sink to something that we opened write-only
mikelutz (he/him) I do really feel that the documentation in the ‘sink’ and ‘stream’ options sufficiently describes what’s happening here.
benjifisher $response = $client->get($source, $options)->getBody();
Where in the documentation you quoted does it say that $response will be a temp stream unless $options['sink'] is set?
mikelutz (he/him)
/** * sink: (resource|string|StreamInterface) Where the data of the * response is written to. Defaults to a PHP temp stream. Providing a * string will write data to a file by the given name. */const SINK = 'sink';</td></tr>
<tr><td>mikelutz (he/him)</td><td>That’s the documentation for $options[‘sink’]
benjifisher I have looked at that.
benjifisher Maybe what I am missing is the explanation that this is what getBody() returns.
mikelutz (he/him) I feel like that is implied with/** * stream: Set to true to attempt to stream a response rather than * download it all up-front. */const STREAM = 'stream';
benjifisher It is not clear to me from reading that.
mikelutz (he/him) I read is as “Set $options[‘stream’] to true to get back the original download stream, otherwise, we will dump that to a local temp stream, unless you override that temp stream with another stream with $options[‘sink’]”
benjifisher In fact, I still do not see what stream has to do with it. Is that one of the options?
benjifisher Oh, it is the (boolean?) option that we do not set?
mikelutz (he/him) It’s an option we aren’t using, correct.
mikelutz (he/him) You’ve been saying that when we set ‘sink’ we get the destination stream in getBody instead of the source stream, which is confusing.
mikelutz (he/him) I’m saying we ALWAYS have gotten a ‘destination’ stream it’s just usually a read/write temp stream.
benjifisher I looked at the doc blocks in Guzzle. I did not see a lot of @return comments.
The relation between what is returned by these methods and what is downloaded is not clear to me.
Yes, I am professing ignorance. Documentation is written for the sake of the ignorant!
mikelutz (he/him) $options[‘stream’] which is never used in Drupal would be the only way to get a ‘source’ stream.
mikelutz (he/him) I’m with you that the documentation is not great at all.  I had to look through code to understand that this is what the limited docs were implying.
benjifisher What if we switch from Guzzle to Gazzle? Is another PSR7 implementation guaranteed to work the same way?
mikelutz (he/him) My main point is that it’s not guzzle returning an unreadable destination stream because we used ‘sink’ it’s guzzle returning the ‘sink’ stream like always, we just overrode the default sink with an unreadable stream. (edited)
benjifisher OK, I follow that.
mikelutz (he/him) So we deal with an unreadable stream in getBody, or we open our sink stream r+ so we can read it like we normally would the temp stream. I don’t know if we really want to open the file r+ or not really, but those are the two options.
mikelutz (he/him) Well, psr 7 only defines the response interface. The options we are using (‘sink’) is part of Guzzle, not PSR 7. So we couldn’t guarantee Gazzle has a sink option.
mikelutz (he/him) and the response interface psr7 defines only requires ->getBody to return a StreamInterface. in that context, (if ->isReadable read, else don’t) is fine.
mikelutz (he/him) The client is defined in psr-18, and I don’t know if we are on a psr-18 compliant guzzle yet. (I should know this, I opened an issue for it)
benjifisher Gazzle is less realistic than the next major release of Guzzle, which may or may not be psr-18 compliant. Do we know that it will behave the same way? That is, get(...)->getBody() is supposed to return the destination stream, which defaults to a RW temp stream?
mikelutz (he/him) The more pertinent question, is does the next client support a ‘sink’ option at all.
mikelutz (he/him) PSR-18 doesn’t really define any ability to use options at all.
mikelutz (he/him) but $response->getBody() is guaranteed to be a stream containing the response body. It’s not guaranteed to be any specific stream containing the response body.
mikelutz (he/him) even in our download plugin, it’s a stream containing the response body.
benjifisher As I said, if you are still not convinced then I will upload a comment-only patch.
mikelutz (he/him) we just happened to override that stream to something we can’t read.. oops.
benjifisher I think the download plugin is sensible: why should we need to make the download stream readable? IMO the "oops" is in the testing system.
mikelutz (he/him) I think we should encourage improved upstream documentation, but no, I don’t believe that test belongs in core.  With everything I’ve figured out looking at this, “Testing system tries to save the wrong stream when using Guzzle” isn’t even right. The testing system is trying to save the stream we told guzzle to use.
mikelutz (he/him) The comment should say something more along the lines of “The stream may not be readable, i.e. in the case of sinking the body directly into a write-only file stream” (edited)
benjifisher The stream. not the file, is write-only.
mikelutz (he/him) good point, that’s true.
benjifisher I think the comment is accurate
:              $stream = $response->getBody();              // Get the response body as a string. If the request is sent with
              // $options['sink'] set, then $stream is set to $options['sink'],
              // which may not be readable.

(edited)

benjifisher I think that is consistent with everything you said in this thread.
mikelutz (he/him) It is but I prefer phrasing that doesn’t make it sound like guzzle is doing something abnormal. we aren’t setting $options[‘sink’], we are overriding the default.
mikelutz (he/him) “If the default sink stream has been overridden, then it may not be readable”
mikelutz (he/him) So the next person realizes that ->getBody() is ALWAYS a “destination” stream
mikelutz (he/him) or at least is by default.
benjifisher I am struggling with the temptation to explain too much. We do not want a 20-line comment here. ...
mikelutz (he/him) No, the important thing is we learned something today. The fewer people that know, the better our job security. :stuck_out_tongue:
benjifisher You mean our volunteer jobs as Drupal maintainers?
I was once fired from a volunteer job ...
mikelutz (he/him) Hey, my volunteer job as a maintainer looks really good on a resume, and it contributed to me getting my paid job…
mikelutz (he/him) Now, the better question is why did the original test failure show up suddenly, and why wasn’t Adam or I able to trigger it?  did something change in casting a write-only stream to a string? Is there a setup where that works and returns ‘’ and another where it doesn’t?
mikelutz (he/him) That I still don’t know. From what I learned today if feels like that should have always failed.
benjifisher One guess I had is that you and Adam set up your tests to use something like a RAM disk. That is, an in-memory filesystem. And this hypothetical optimization generates streams that are always RW.
benjifisher Before I remove the test from the MR, you could run it locally. That might be interesting.
mikelutz (he/him) Maybe, but testbot started failing on a guzzle update.
benjifisher True. I think that is a separate question.
benjifisher Does @ suppress exceptions as well as errors?
mikelutz (he/him) I don’t think so.
mikelutz (he/him) Guzzle does wrap the file resource stream in a psr StreamInterface stream.  Maybe something changed there.
mikelutz (he/him) I’d have to look at the git history of that section of guzzle, and I’m not going to do it tonight.
benjifisher I think @quietone already did that, on the original issue: https://github.com/guzzle/psr7/pull/515/files
mikelutz (he/him) I could track it down if I could trigger the old error and bisect guzzle, I suppose, but that’s a project for another day.
benjifisher If Guzzle previously generated an error when you try to cast an unreadable stream to string and now throws an exception instead, then that would explain why @(string) stopped hiding the mistake.
mikelutz (he/him) Ah, that’s what changed. It used to throw a warning, and return false, which would be castable to a string. now it throws an exception.
mikelutz (he/him) Still don’t know why I didn’t trigger it though, I shouldn’t be using a ram filesystem.
mikelutz (he/him) I guess maybe I am… my sites/simpletest folder that used to hold files for functional tests doesn’t have any directories newer than 2020.
mikelutz (he/him) dunno when or what changed that, but I’m okay with it.
benjifisher I know that people who care run with SQLite using a "RAM disk". I thought there might be something similar for fileaccess.
mikelutz (he/him) I don't know where that would be. I run tests on my local mysql
benjifisher An easy way to check is by running the test you want me to nix. If it tells you that the stream URI is the expected one, but that it is unexpectedly readable ...
mikelutz (he/him) I couldn't pause the test at that logging line either, but I think that's about my xdebug setup and functional tests.
benjifisher If the test fails, it will tell you.
mikelutz (he/him) I’ll try
mikelutz (he/him) nope, it passed for me.
mikelutz (he/him) No clue, but I’m done braining tonight. it’s beer oclock.
benjifisher Word smithing: how do you like this?
              // Get the response body as a string. The response stream is set
              // to the sink, which defaults to a readable temp stream but can
              // be overridden by setting $options['sink'].
mikelutz (he/him) Yeah, that’s better.
quietone Nice read. Thanks!

1️⃣1️⃣ Wrap up

benjifisher Thanks for participating! I will update 2️⃣. Please continue to add comments in the threads. In 1-7 days, we will post a transcript for today's meeting.

Comments

quietone created an issue. See original summary.

quietone credited mikelutz.

quietone’s picture

Issue summary: View changes
Status: Active » Needs review
quietone’s picture

Status: Needs review » Fixed

No request for changes at following meeting, closing.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.