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

Comments

voleger created an issue. See original summary.

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
CreditAttribution: voleger as a volunteer and at GOLEMS GABB, Drupal Ukraine Community for Drupal Ukraine Community, GOLEMS GABB commented
FileSize
3134540-2.patch1.42 KB

Here the patch

alex_optim’s picture

alex_optim
Location 🇺🇦 Rivne
CreditAttribution: alex_optim at GOLEMS GABB, Drupal Ukraine Community for GOLEMS GABB, Drupal Ukraine Community commented
Status: Active » Reviewed & tested by the community

Looks good.

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
CreditAttribution: voleger as a volunteer and at GOLEMS GABB, Drupal Ukraine Community for Drupal Ukraine Community, GOLEMS GABB commented

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

pifagor’s picture

pifagor
Location 🇺🇦 Rivne
CreditAttribution: pifagor at Drupal Ukraine Community, GOLEMS GABB for Drupal Ukraine Community, GOLEMS GABB 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
Location 🇺🇦 Rivne
CreditAttribution: pifagor at Drupal Ukraine Community, GOLEMS GABB for Drupal Ukraine Community, GOLEMS GABB 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
CreditAttribution: Dave Reid as a volunteer and at Lullabot 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
CreditAttribution: Dave Reid as a volunteer and at Lullabot 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
CreditAttribution: Dave Reid as a volunteer and at Lullabot commented
Status: Needs work » Needs review
FileSize
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
CreditAttribution: Dave Reid as a volunteer and at Lullabot commented
Status: Needs work » Needs review
FileSize
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
CreditAttribution: Dave Reid as a volunteer and at Lullabot 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.