Problem/Motivation

When #2408013: Adding Assertions to Drupal - Test Tools. is closed as Fixed there is 6 TODOs still in place to replace exceptions with assertions.

$grep -r 2408013 core | awk -F: '{print $1}'| nl
     1  core/lib/Drupal/Core/Entity/ContentEntityBase.php
     2  core/lib/Drupal/Core/Ajax/AjaxResponseAttachmentsProcessor.php
     3  core/lib/Drupal/Core/Plugin/Context/LazyContextRepository.php
     4  core/lib/Drupal/Core/Render/Renderer.php
     5  core/lib/Drupal/Core/Render/Renderer.php

Proposed resolution

Replace all mentioned exceptions with assertions.

Remaining tasks

  1. Decide on whether we want a separate issue per exception or doing it all here would be fine.
  2. Patch
  3. Review
  4. Commit

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

CommentFileSizeAuthor
#28 3081646-nr-bot.txt90 bytesneeds-review-queue-bot

Issue fork drupal-3081646

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

RoSk0 created an issue. See original summary.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

richardbporter’s picture

richardbporter commented
Assigned: Unassigned » richardbporter

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone commented
Issue summary: View changes

Make it easier to read the list of files in the IS.

mallezie’s picture

mallezie
Primary language Dutch
Location Loenhout
commented
Issue summary: View changes

The one in core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php is already done.
The ones in the Renderer class are being tackled in https://www.drupal.org/project/drupal/issues/3192839

I rebased the branch, and added changes for the other 3 to see if we do them in one go.

mallezie’s picture

mallezie
Primary language Dutch
Location Loenhout
commented
Assigned: richardbporter » Unassigned

mallezie opened merge request !1179

mallezie’s picture

mallezie
Primary language Dutch
Location Loenhout
commented
Status: Active » Needs review

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

At this time we will need a D10 MR of this issue. Would recommend starting a new one as I've experienced issues with updating an MR from 9.x to 10.

Spokje made their first commit to this issue’s fork.

Spokje opened merge request !3281

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review

Thanks @smustgrave and The Kind Folks @#needs-review-queue-initiative

Rerolled in a new MR against 10.1.x.

mallezie closed merge request !1179

mallezie’s picture

mallezie
Primary language Dutch
Location Loenhout
commented

Thanks! Totally forgot about this one.

Just checked the new MR, and seems the same to the original. (Which i did, so probably i'm not the one allowed to put it in RTBC).

Closed the old merge request.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Thanks for the turnaround

ON Drupal 10.1.x
Did a quick search for https://www.drupal.org/node/2408013 and found 4 instances
Applied the MR and they were addressed.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: +Needs framework manager review

Tagging for framework manager review since this caused its share of debate back in the day.

spokje’s picture

spokje
Primary language Dutch
commented

Am I just unlucky, or is every issue I worked on lately needing policy updates/[Important-People] review?

xjm’s picture

xjm
she/her
Primary language English
commented

@Spokje I think that means you are working on important things. :)

VladimirAus made their first commit to this issue’s fork.

catch’s picture

catch
he/him
Primary language English
commented

fwiw these all look like good changes to me - they're notifying of developer errors, which is the right place for an assert.

The only one that had me scratching my head was saving an entity without validation, since the exception potentially prevents bad data being saved, but that also looks like a situation you can only get into via developer error so the assertion should still catch it before it actually happens on production anywhere.

spokje’s picture

spokje
Primary language Dutch
commented

@Spokje I think that means you are working on important things. :)

To boldly go stupidly stumble where no person has gone before!

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3081646-nr-bot.txt90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Reviewed & tested by the community

_Very_ unsure if this is allowed, but since this was a reroll and not done by myself, I'm going t put this back to RTBC per #21

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs framework manager review

Agreed we should do this, there are probably more examples but we can gradually open issues for those when we spot them. Untagging for framework manager review.

Also to confirm you're definitely allowed to re-RTBC something that's only been re-rolled since it was last RTBC.

Committed e434d61 and pushed to 10.1.x. Thanks!

  • catch committed e434d61d on 10.1.x 
    Issue #3081646 by Spokje, mallezie, richardbporter, VladimirAus, RoSk0:...
catch’s picture

catch
he/him
Primary language English
commented
spokje’s picture

spokje
Primary language Dutch
commented

Also to confirm you're definitely allowed to re-RTBC something that's only been re-rolled since it was last RTBC.

Thanks @catch, there's probably a very clear policy out there, but my search-fu failed me on this one.
Very handy to know for the future.

Status: Fixed » Closed (fixed)

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

Spokje closed merge request !3281

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented

Adjusting issue credit as credit is not provided for button-click rebases without additional contribution.

andypost’s picture

andypost
he/him
Primary language Russian
commented

This fix caused issue with PHP 8.3 compatibility #3375693-14: Fix deprecated assert_options() function usage for PHP 8.3

Let's polish assertions there