Followup for #3099673-8: Replace deprecated file_scan_directory()

CommentFileSizeAuthor
#13 3134540-throw-exception-when-directory-not-ready.patch5.78 KBdave reid
#11 3134540-throw-exception-when-directory-not-ready.patch3.46 KBdave reid
#2 3134540-2.patch1.42 KBvoleger

Comments

voleger created an issue. See original summary.

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
commented
StatusFileSize
new 3134540-2.patch1.42 KB

Here the patch

alex_optim’s picture

alex_optim
Location Tallinn
commented
Status: Active » Reviewed & tested by the community

Looks good.

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
commented

#3134571: Drupal 9 compatibility filled to address branch Drupal 9 compatibility issues

pifagor’s picture

pifagor commented

  • pifagor committed ca43ebd on 8.x-1.x authored by voleger 
    Issue #3134540 by voleger, alex_optim, pifagor: Add exception handling...
pifagor’s picture

pifagor commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Any idea why this was added? The issue here is scant on details.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Closed (fixed) » Needs work

If the directory doesn't exist, doesn't it mean that the sitemap file writing process also failed? Why wouldn't we want to expose a failure here instead of letting it silently fail?

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs work » Needs review
StatusFileSize
new 3134540-throw-exception-when-directory-not-ready.patch3.46 KB

I think this would be better, we should throw an exception when xmlsitemap_check_directory() fails, instead of silently letting it pass through, which I think is the issue here. Because then nothing in the generation process would actually work if the directory didn't exist.

Status: Needs review » Needs work

The last submitted patch, 11: 3134540-throw-exception-when-directory-not-ready.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs work » Needs review
StatusFileSize
new 3134540-throw-exception-when-directory-not-ready.patch5.78 KB

I've updated xmlsitemap_sitemap_get_directory() to always return a valid value, so the test that sets the directory to empty is no longer valid and is removed. It shouldn't be a possible condition since the config is required.

  • Dave Reid committed fde4704 on 8.x-1.x 
    Issue #3134540 by Dave Reid: Fixed sitemap generation should fail if...
dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs review » Fixed

Tested and committed #13 to 8.x-1.x.

Status: Fixed » Closed (fixed)

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