I think for this one we need a field.yml config file as well?

CommentFileSizeAuthor
#8 1798782-9.patch2.6 KBswentel
#5 1798782_field_purge_batch_size_3.patch734 bytesandreiashu
#2 1798782_field_purge_batch_size_1.patch710 bytesandreiashu
#3 1798782_field_purge_batch_size_2.patch746 bytesandreiashu
#1 1798782_field_purge_batch_size_1.patch710 bytesandreiashu

Comments

andreiashu’s picture

andreiashu commented
Status: Active » Needs review
StatusFileSize
new 1798782_field_purge_batch_size_1.patch710 bytes
andreiashu’s picture

andreiashu commented
Status: Needs review » Needs work
StatusFileSize
new 1798782_field_purge_batch_size_1.patch710 bytes

tagging and back to needs work

edit: not sure how the attached patch for in this comment...

andreiashu’s picture

andreiashu commented
Issue tags: +Configuration system
StatusFileSize
new 1798782_field_purge_batch_size_2.patch746 bytes

YML file is now called field.settings.yml, updated code as well

yched’s picture

yched commented
Status: Needs work » Needs review

We don't need 'field_' in the name of the new property - 'purge_batch_size' should be enough.
Other than that, this should be ready to fly.

andreiashu’s picture

andreiashu commented
StatusFileSize
new 1798782_field_purge_batch_size_3.patch734 bytes

thanks for the review Yves. New patch attached

yched’s picture

yched commented
Status: Needs review » Reviewed & tested by the community

Thanks !

dries’s picture

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

Committed to 8.x. Thanks.

swentel’s picture

swentel commented
Status: Fixed » Needs review
StatusFileSize
new 1798782-9.patch2.6 KB

Sorry te reopen, but the patch missed a couple of field_purge_batch functions which still are hardcoded to 10.

Stalski’s picture

Stalski commented
Status: Needs review » Reviewed & tested by the community

Improvements are fine

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Should the parameter to field_purge_batch be optional, and default to this config value, rather than having to repeat config()->get() everywhere?

swentel’s picture

swentel commented

Sounds plausible, there's no UI for this one anyway and probably never will be either. I can live with both situations, let's wait until a core committer checks the patch.

yched’s picture

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

Nope, the hardcoded 10s that remain are intentional. They didn't read from the variable before, ee still want them to be 10 even if someone puts the config value to 1000

swentel’s picture

swentel commented
Status: Fixed » Closed (fixed)

Oh, alright, sorry for the noise :)