Skip to content

Remove invalidate_cached_urls.#1998

Merged
parndt merged 1 commit into
2-0-stablefrom
remove_invalidate_cached_urls
Nov 9, 2012
Merged

Remove invalidate_cached_urls.#1998
parndt merged 1 commit into
2-0-stablefrom
remove_invalidate_cached_urls

Conversation

@ugisozols

Copy link
Copy Markdown
Member

It's not needed because:

  • cache is cleared by after_filter in admin pages controller
  • when rebuild is called it runs #invalidate_cached_urls on every page
    and its children so in case user has many pages it causes a lot of
    queries which increases load time and sometimes even time out.

See #1954 for discussion.

@parndt

parndt commented Oct 24, 2012

Copy link
Copy Markdown
Member

Not too big of a change for 2-0-stable?

@ugisozols

Copy link
Copy Markdown
Member Author

It's a private api so in theory it should be safe to remove/change.

Other option (probably a bit safer since we don't have specs) is to just remove find_each call in https://github.com/refinery/refinerycms/blob/2-0-stable/pages/app/models/refinery/page.rb#L193 or remove https://github.com/refinery/refinerycms/blob/2-0-stable/pages/app/models/refinery/page.rb#L191-195 altogether since there's no point to keep it in case we remove find_each.

@Matho

Matho commented Oct 24, 2012

Copy link
Copy Markdown
Contributor

We don't need to call rebuild! from awesome-nested-set ? (that alias_method_chain ) #1954 (comment) When I was testing it, my tree was allways valid, so it returned true. But is there any situations, when tree is not valid and rebuild! should run?

@ugisozols

Copy link
Copy Markdown
Member Author

awesome_nested_set checks if tree is invalid and only rebuilds if it's invalid. See https://github.com/collectiveidea/awesome_nested_set/blob/master/lib/awesome_nested_set/awesome_nested_set.rb#L184-185.

Tbh I have no idea when tree can become invalid (maybe if you do some manual record editing in db or something like that).

@parndt

parndt commented Oct 30, 2012

Copy link
Copy Markdown
Member

I think that it's safer to merge any of this into master not 2-0-stable branch. I don't consider any of this a bug fix.

If anybody disagrees please let me know.

@parndt parndt closed this Oct 30, 2012
@ugisozols

Copy link
Copy Markdown
Member Author

This code isn't present in master so it only applies to 2-0-stable.

I can agree that it's safer not to merge this in 2-0-stable but we need to do something about #1954 since it's a real issue.

@parndt

parndt commented Oct 30, 2012

Copy link
Copy Markdown
Member

Can we just stop calling it?

@parndt parndt reopened this Oct 30, 2012
@parndt

parndt commented Oct 30, 2012

Copy link
Copy Markdown
Member

@ugisozols can you please document this in the changelog?

ugisozols added a commit that referenced this pull request Oct 31, 2012
@ugisozols

Copy link
Copy Markdown
Member Author

@parndt done ^

@parndt

parndt commented Oct 31, 2012

Copy link
Copy Markdown
Member

Another PR in here too? :-)

@ugisozols

Copy link
Copy Markdown
Member Author

I actually don't get it why it's here because I only rebased against remove_invalidate_cached_urls...

@ugisozols

Copy link
Copy Markdown
Member Author

@parndt I squashed everything into one commit.

@robyurkowski

Copy link
Copy Markdown
Contributor

@ugisozols does this need verification, or can it be closed, then?

@ugisozols

Copy link
Copy Markdown
Member Author

If you're ok with this change please merge it.

It's not needed because:

* cache is cleared by after_filter in admin pages controller
* when rebuild is called it runs #invalidate_cached_urls on every page
and its children so in case user has many pages it causes a lot of
queries which increases load time and sometimes even time out.

See #1954 for discussion.
@parndt parndt merged commit 7f77e34 into 2-0-stable Nov 9, 2012
@parndt

parndt commented Nov 9, 2012

Copy link
Copy Markdown
Member

Sure thing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants