🇧🇷Brazil @lucassc

Providing an interdiff between the patch in #53 and the patch before this in #48.

Marking as RTBC if no one else disagrees.

Removing typo.

Adding before/after screenshots in the IS.

Updating screenshots for IS.

Thanks, @smustgrave. I replaced the remaining 2 "illegal choice" recurrences.

Please review.

could someone please provide before/after screenshots and steps to reproduce in the IS?

ops, damn ADHD haha

extra full stops removed.

Hi, nicrodgers! Thanks for review.

In #11 I already added an alternative patch without the commas and its interdiff with patch #9.

Please, review.

Hi!

I removed the remaining ambiguous messages and I took the liberty of include a comma after the word "Please".

I attached an alternative patch with no commas as well.

Please, review.

Hi, @longwave! Thanks for the code review.

I removed the unnecessary casts and followed your suggestions rewriting some lines in a cleaner way.

Just not sure about this change in core/lib/Drupal/Core/Updater/Updater.php:

$old_perms = fileperms($parent_dir) & 0777;
...
$filetransfer->chmod($old_perms);

Should we keep with '0755' as the 2nd argument?
$filetransfer->chmod($old_perms, 0755);

Hi, @PrabuEla!

Thanks for reply. You're right, it's optional to replace it nowadays but I think it's good to unify the way in core - as it was said in the parent issue. 📌 Replace intval(), strval(), .. *val() functions with their relevant type casting operators Postponed

I'm not sure about the performance improvement matter, but I think it might still be relevant?

Hi, Liam!

Could you please provide some documentation or steps to reproduce?

Rerolled for 7.x-4.x.

Please, review.

Patch #2 rerolled. I'm not attaching the interdiff because there are no differences between one patch and another, but it's only changing 1 line, so you can easily compare with the patch in #2.

Please, review.

Addressed to #29. Please, review.

Follow-up issues created: 📌 Replace Updater.php intval() functions with their relevant type casting operators Fixed and 📌 Replace the use of intval() and strval() as callbacks with a foreach and typecast or such Postponed: needs info .

I guided myself with this doc → , as it's the first time I've done it. I hope that's correct.

I believe the next step is to update the issue summary? Tagging for this.

lucassc → created an issue.

lucassc → created an issue.

We had comments in the old MR that still need to be discussed.

core/lib/Drupal/Core/Entity/Query/Sql/Query.php:

// Add a self-join to the base revision table if we're querying only the
// latest revisions.

@daffie said "This comment needs an update as it no longer reflects what we are doing."

core/tests/Drupal/KernelTests/Core/Entity/EntityQueryTest.php:

$results = $this->queryResults = $this->storage
  ->getQuery()
  ->latestRevision()
  ->notExists("$figures.color")
  ->accessCheck(TRUE)
  ->execute();
$expected = [16 => '4', 8 => '8', 20 => '12'];
$this->assertSame($expected, $results);

// Update an entity.
$entity = EntityTestMulRev::load(4);
$entity->setNewRevision();
$entity->$figures->color = 'red';
$entity->save();

$results = $this->queryResults = $this->storage
  ->getQuery()
  ->latestRevision()
  ->notExists("$figures.color")
  ->accessCheck(TRUE)
  ->execute();
$expected = [8 => '8', 20 => '12'];
$this->assertSame($expected, $results);

@daffie said: "We need to do a change that keeps it in the result and therefore we see the changed revision id. The current change removes it from the result."

@neclimdul replied "I'm not sure what this is asking. What result would we be able to see the revision ID in?"

Hi! I opened a MR for 10.1.x, which was not available first from the "Create new branch" link at the top of the issue. So I followed this doc → to add 10.1.x as the basis for my issue branch 2950869-optimizing-revision-queries.

Not sure if this is the correct way, I'm novice.

Please, review.

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

Unnecessary parentheses removed. Please, review.

Should we aggregate the changes in the comments from patch #27?

Patch to make $modules protected. Please, review.

Comments added. Please, review.

pameeela → credited lucassc → .

I agree, that's right.

The new description sounds good to me! It's clear enough about what the "default front page" actually is. +1

That's right! @size's arguments removed. I took the opportunity to add a final point that was missing.

And what about these comments?

// Render a nice error message in case we have a file upload which exceeds
// the configured upload limit.

Should we change them too?

// An error message should appear informing the user that the file exceeded
// the maximum file size. The error message includes the actual file size
// limit which depends on the current environment, so we check for a part
// of the message.

My bad, I couldn't attach an interdiff duo to the error below:

1 out of 1 hunk FAILED -- saving rejects to file /tmp/interdiff-1.BEe0qz.rej
interdiff: Error applying patch1 to reconstructed file

But the only difference was that I kept the new D10 statusMessageContainsAfterWait(message, 'error') over the patch's pageTextContains.

Rerolled for 10.1.x

I agree that "default front page" often causes a lot of confusion for those who start learning Drupal with the Standard installation profile, which set the default front page to /node.

But I'm not sure about simply adding "(/user/login)" after "default front page" in the sentence, it doesn't seem good enough regarding user experience. For those who start with the Standard installation profile, the default value in "Default front page" field indeed starts with /node. In this case, the behavior expected by the user when leaving the field blank is the default value /node (as the site started). "Whenever possible, build something that feels familiar. (...) If something seems unconventional, flag it and consider whether it should actually appear and work that way", right?

Maybe some more additional information about use "/node" to revert to Standard installation profile's default front page /node? Maybe we can add an info icon and help text to explain about everything: the default front page /user/login, which redirects to the user profile page for logged in users, and the /node page for Standard installation profile.

Is there any way to get the default front page value directly from system.site.yml?

Tagging "ux" to help us with this.