Skip to content

add isNewSite to ResaveElements job#16924

Merged
brandonkelly merged 7 commits into4.xfrom
bugfix/resaving-element-for-new-site
Mar 20, 2025
Merged

add isNewSite to ResaveElements job#16924
brandonkelly merged 7 commits into4.xfrom
bugfix/resaving-element-for-new-site

Conversation

@i-just
Copy link
Copy Markdown
Contributor

@i-just i-just commented Mar 19, 2025

Description

Same fix as here but for when we’re adding site to a section, so that this code doesn't kick in.

Steps to reproduce:

  • clean 5.6.12 install
  • create 2 sites
  • create a section relations with an entry type that can have just the default title; enabled for both sites
  • create at least one entry in the relations section
  • create an Entries field, non-translatable
  • create a section blog with an entry type that contains that entries field added twice; the section should only be enabled for the primary site
  • create an entry in the blog section, fill out the title and fully save
  • edit the entry from the previous step, add an entry to the second Entries field & save
  • edit the blog section and enable it for the second site
  • wait for the resave job to complete
  • view the blog entry in the primary site - all is good; switch to the second site - the relation is showing in the first field, not the second one; if you check the elements_sites table for this elementId, the content column will be null for the site we just enabled for this section.

I wasn’t able to reproduce it on v4, but since we previously added this mechanism for v4, I opted to do this bit here. If it was a wrong call, LMK and I can target only v5.

Related issues

n/a; noticed while working on #16919

@i-just i-just requested a review from brandonkelly March 19, 2025 15:54
@brandonkelly
Copy link
Copy Markdown
Member

brandonkelly commented Mar 19, 2025

Doesn’t look like this fix is complete? ResaveElements::$isNewSite is unused.

Assume its value needs to be passed onto $item->isNewForSite within processItem()?

Let’s have the property be renamed to $isNewForSite too – since in this case the site itself isn’t new; it’s just new for the elements being resaved.

@i-just
Copy link
Copy Markdown
Contributor Author

i-just commented Mar 20, 2025

Yeah, sorry, my bad. I played with the resaving attribute and didn’t redo all the changes :/

I opted for ResaveElements::$hasNewSite, which is then passed to $item->isNewSite.

The Element::$isNewForSite is already used to check if the element is new for the site we’re working on, and the condition we want to bypass kicks in when Element->isNewForSite is true, so I didn’t want to mess with it.

Alternatively, this could use the Element::$resaving property with a check for it added here, but it would change how the resave job behaves at the moment, so I’m not sure it’s a good idea.

@brandonkelly
Copy link
Copy Markdown
Member

brandonkelly commented Mar 20, 2025

Alternatively, this could use the Element::$resaving property with a check for it added here, but it would change how the resave job behaves at the moment, so I’m not sure it’s a good idea.

I do think that’s actually a better option here. Went ahead and made that change.

@brandonkelly brandonkelly merged commit 2ffa321 into 4.x Mar 20, 2025
4 checks passed
@brandonkelly brandonkelly deleted the bugfix/resaving-element-for-new-site branch March 20, 2025 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brandonkelly brandonkelly Awaiting requested review from brandonkelly

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@i-just @brandonkelly