Lots more exception throwing and catching is needed throughout the code base, especially for all simple_sitemap API methods.

CommentFileSizeAuthor
#19 simple_sitemap-deleted_entity-3027987-18.patch3.64 KBGrimreaper
#18 preview_link-multilingual_support-3124785-18.patch4.98 KBGrimreaper
#15 simple_sitemap-deleted_entity-3027987-15.patch2.64 KBGrimreaper
#10 simple_sitemap-deleted_entity-3027987-10.patch2.85 KBGrimreaper
#6 entity_object_check_3027987-4.patch1.07 KBraselkabir
#5 entity_object_checky_3027987-3.patch978 bytesraselkabir
#2 entity_object_checky_3027987-2.patch875 bytesjoshi.rohit100

Issue fork simple_sitemap-3027987

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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joshi.rohit100 created an issue. See original summary.

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 at Acquia commented
Status: Active » Needs review
FileSize
entity_object_checky_3027987-2.patch875 bytes

Patch added.

gbyte’s picture

gbyte CreditAttribution: gbyte as a volunteer and at gbyte commented
Title: Simplesitemap::setEntityInstanceSettings() loads and uses entity without actual check if really an entity object » Throw and Catch exceptions in API
Version: 8.x-2.x-dev » 8.x-3.x-dev
Category: Bug report » Task
Status: Needs review » Active

While this is a valid objection, it's not a bug, as you should do checks anyway when using the api directly.

Also this kind of improvements only end up in 3.x, as 2.x moves towards deprecation.

It would be beneficial to find all the places in the code where entities are loaded without that check. Lots more exception throwing and catching is needed throughout the code base. Looking forward to a patch!

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 at Acquia commented

@gbyte.co I raised for 2.x as currently we using this.

Regarding the check, the scenario was like that we called this api in custom code while deleting nodes. So somehow there was exception (deadlock) and node deleted partially. But this code still executed without checking object.

raselkabir’s picture

raselkabir CreditAttribution: raselkabir at Digitalist commented
Version: 8.x-3.x-dev » 8.x-3.2
FileSize
entity_object_checky_3027987-3.patch978 bytes

Same problem for getEntityInstanceSettings also. So I am adding a patch for both.

raselkabir’s picture

raselkabir CreditAttribution: raselkabir at Digitalist commented
Status: Active » Needs review
FileSize
entity_object_check_3027987-4.patch1.07 KB

Same problem for getEntityInstanceSettings also. So I am adding a patch for both.

Status: Needs review » Needs work

The last submitted patch, 6: entity_object_check_3027987-4.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 at Acquia commented

Setting as NW as this require try/catch instead entity object check.

gbyte’s picture

gbyte CreditAttribution: gbyte as a volunteer and at gbyte commented
Version: 8.x-3.2 » 8.x-3.x-dev
Issue summary: View changes

I agree and committing a community provided patch adding exceptions to the API (and preferably elsewhere) would make my day.

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper at Smile commented
Status: Needs work » Needs review
FileSize
simple_sitemap-deleted_entity-3027987-10.patch2.85 KB

Hello,

I also encountered the problem when dealing with deleted entities.

Here is a patch that is more complete than patch from comment 6.

I have search in the module codebase 'load'.

Thanks for the review.

johnpicozzi’s picture

johnpicozzi
Primary language English
Location Providence, RI
CreditAttribution: johnpicozzi at Oomph, Inc. commented

I was having the issue described here - https://www.drupal.org/project/simple_sitemap/issues/3066138

The patch in #10 above resolved the issue for me.

gbyte’s picture

gbyte CreditAttribution: gbyte as a volunteer and at gbyte commented
Status: Needs review » Needs work

@Grimreaper Please refer to #8. When using the API, we need sensible exceptions and not for the code to silently fail when an entity does not exist.

@johnpicozzi Try the latest dev, there have been some chanes in terms of checks when using UI. For raw API, you still need to make sure entities exist.

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper at Smile commented

@gbyte.co The load() method on EntityStorageInterface does not trigger exception in case of error. So you mean triggering custom exceptions? Or before returning logging an error?

johnpicozzi’s picture

johnpicozzi
Primary language English
Location Providence, RI
CreditAttribution: johnpicozzi at Oomph, Inc. commented

The patch in #10 doesn't apply to 8.x-3.4, looks like it needs to be re-rolled.

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper at Smile commented
FileSize
simple_sitemap-deleted_entity-3027987-15.patch2.64 KB

Here is a patch for the last dev version (8.x-3.5 +1 commit).

gbyte’s picture

gbyte CreditAttribution: gbyte as a volunteer and at gbyte commented

Here is a patch for the last dev version (8.x-3.5 +1 commit).

Your changes add additional class checking on top of the entity exists check, but the class check should not be necessary here, as only content entities are allowed in the queue anyway.

The load() method on EntityStorageInterface does not trigger exception in case of error. So you mean triggering custom exceptions? Or before returning logging an error?

API methods where the API is being used incorrectly should trigger a custom verbose exception; Drupal will log it automatically.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

I just wanted to mention that I was having a lot of problems with the 8.x-3.6 release on a test infrastructure, the latest dev snapshot has resolves these and is working really well for us. Good work everyone!

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper at Smile commented
Status: Needs work » Needs review
FileSize
preview_link-multilingual_support-3124785-18.patch4.98 KB

Hello,

Here is an updated patch because patch from comment 15 no more apply on 8.x-3.7.

I hope it takes comment 16 into account otherwise I don't see what is expected in this comment.

Regards,

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper at Smile commented
FileSize
simple_sitemap-deleted_entity-3027987-18.patch3.64 KB

Oups, wrong patch...

gbyte’s picture

gbyte CreditAttribution: gbyte as a volunteer and at gbyte commented
Version: 8.x-3.x-dev » 4.x-dev
Assigned: Unassigned » gbyte
Status: Needs review » Active
Parent issue: » #3219383: Roadmap for 4.x

I am addressing this in the 4.x rewrite of this module.

  • gbyte committed 0724121 on 4.x 
    Issue #3027987 by gbyte: Throw and Catch exceptions while generating

Grimreaper opened merge request !21

gbyte’s picture

gbyte CreditAttribution: gbyte as a volunteer and at gbyte commented
Status: Active » Fixed

Status: Fixed » Closed (fixed)

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