Problem/Motivation
In #2260187: [PP-1] Deprecate drupal_static() and drupal_static_reset() we'll deprecate the drupal_static()
& drupal_static_reset()
procedural functions. However, in order to do so, we have to remove first all usages from Drupal core.
Proposed resolution
We cannot do all the removals in one step. Each usage it's an issue itself. So, this issues is a META to track the progress:
Child issues:
- #3035288: Deprecate theme_get_setting()
- #3035340: Deprecate views_ui_add_ajax_trigger()
- #2875151: [META] Implement Batch API as a service, #2875219: [PP-1] Batch not using new service to cover the usage in
_batch_needs_update()
. - #2324719: Node indexing - should use view mode for comments, not hook to cover the usage in
comment_node_update_index()
. - #3035343: Deprecate drupal_attach_tabledrag(). Move its logic in Table form element to cover the usage from
drupal_attach_tabledrag()
. - #3037054: Deprecate drupal_static_reset() without @trigger_error() usage from
drupal_flush_all_caches()
. This is tricky! Probably we want to deprecatedrupal_static()
only when writing to the cache but we still want to allow a non-deprecateddrupal_static_reset()
to clear the static cache. - #3036010: Fold drupal_get_updaters() into Updater class, deprecate drupal_get_updaters() usage from
drupal_get_updaters()
- #3035352: [PP-1] Deprecate file_get_file_references(). Move the logic to file.usage service
- #2536594: Add a FilterFormatRepository providing methods to load filter formats to cover usages from
filter_formats()
andfilter_formats_reset()
. - #3037054: Deprecate drupal_static_reset() without @trigger_error() usage from
FunctionalTestSetupTrait
. This is also tricky for the same reason asdrupal_flush_all_caches()
. - #2081585: [META] Modernize the History module to cover usages in
history.module
. - #3037031: Convert locale.compare.inc to a service
- #2660338: locale_get_plural call in PluralTranslatableString is wrong to cover removal from
locale_get_plural()
- #3037156: Locale import history service. Deprecate locale_translation_get_file_history(), locale_translation_update_file_history(), locale_translation_file_history_delete() to cover removal from
locale_translation_get_file_history()
andlocale_translation_update_file_history()
. - #3036982: Deprecate locale_translation_get_projects() and locale_translation_clear_cache_projects()
- #3037054: Deprecate drupal_static_reset() without @trigger_error() usage from
MigrateExecutable::attemptMemoryReclaim()
. Same asdrupal_flush_all_caches()
. - #3038908: Deprecate node_access_view_all_nodes(). Move its functionality in NodeAccessControlHandlerInterface
- #2171397: Move options_allowed_values() to a method somewhere ? to cover usage from
options_allowed_values()
. - #2083123: Shortcut cleanup: Remove duplicated code and deprecate legacy functions to cover usage from
shortcut.module
. - #3038971: Move system_get_module_admin_tasks() into a service and deprecate it
- #2340341: Move template_preprocess, _template_preprocess_default_variables into services to cover usage from
template_preprocess()
. - #3039248: Deprecate views_ui_contextual_links_suppress(), views_ui_contextual_links_suppress_push(), views_ui_contextual_links_suppress_pop() to cove usage from
views_ui_contextual_links_suppress()
.
Done:
- #3039051: Remove useless static cache reset and route rebuilding from TaxonomyTranslationTestTrait(s) & TaxonomyTermViewTest
- #3035297: Don't use drupal_static in AutoIncrementingTestItem
- #3035345: Remove usage of drupal_static_reset() from ContentTranslationOperationsTest
- #3035347: Remove usage of drupal_static_reset() from ContentTranslationTestBase
- #3035349: Remove usage of drupal_static_reset() from EntityReferenceFieldTranslatedReferenceViewTest
- #3035350: Remove usages of drupal_static() from field_test.module
- #3035361: Remove usages of drupal_static() & drupal_static_reset() from file_test.module
- #3035376: Remove usage of drupal_static() from GetFilenameTest
- #3039081: Remove reset of user_role static cache in UserRoleAdminTest::testRoleWeightOrdering()
- #3036962: Remove usage of drupal_static() from install_profile_info()
- #2044435: Convert pager.inc to a service
- #3035518: Convert ImageEffectsTest & ToolkitTest into Kernel tests
- #2908886: Split off schema management out of schema.inc
- #3075703: Move search text processing to a service
- #3039055: Remove useless reset of taxonomy_term_count_nodes static cache
- #3039074: Remove drupal_static() from _update_manager_(unique_identifier|extract_directory|cache_directory)
- #3039039: Deprecate some procedural functions in taxonomy.module
- #3037202: Remove drupal_static() from node_mark()
- #2412669: Remove drupal_static from BookManager
Remaining tasks
N/A
User interface changes
N/A
API changes
N/A
Data model changes
N/A
Release notes snippet
N/A
Original report
from comment #19 of #1567428: Active trail tests, in order to handle static variables in Drupal 8 we should convert every use of statics to instead be protected properties of proper objects. The result of this effort will be that we properly register objects into the Dependency Injection Container and perhaps do away with drupal_static() and drupal_static_reset().
Comments
Comment #1
cosmicdreams CreditAttribution: cosmicdreams commentedComment #2
Crell CreditAttribution: Crell commentedThis isn't a task unto itself per se, I think, but rather a side-effect of other work. Eg, the menu system needs to be turned into a proper set of objects (after routing it ripped out), and the statics eliminated as part of that. Simply creating objects somewhere that hold statics instead of them being inside functions buys us nothing.
Comment #3
sunHEAD has 93 calls to drupal_static()
Most of them in legacy procedural code that is actively being converted into service classes right now. In essence, those calls should "magically vanish" ;-)
However, for some strange reason, some
drupal_static()
s were taken over as-is into object-oriented code that is clean otherwise. For example,Drupal\Core\Cache\DatabaseBackend
still maintains the state of cache tags viadrupal_static()
.In any case, these
drupal_static()
s (1) prevent us from properly using and leveraging the kernel and (2) harm tests.Comment #4
tim.plunkettSpoke with @alexpott in IRC, he said this did not qualify for a beta or release blocker, but agreed it should be a beta target.
Comment #5
catchThe database backend should get fixed by #918538: Decouple cache tags from cache bins (it should have been converted to a static property of the class initially, but better to fix it properly in that issue).
Comment #6
Berdirstatic does not work because static survives test tearDown(), I tried that :)
Comment #7
sidharrell CreditAttribution: sidharrell commentedchanged issue summary to better reflect how to do the replacement.
Comment #8
andypostSo how this issue fits into 8.0.x?
Comment #9
catchThere's not really anything that can be done in itself here. There are 68 references to drupal_static() left in the code base. This is compared to 191 in Drupal 7.
During the 8.x cycle, we'll be able to move more code to classes/services, which will mean less drupal_static() calls, leaving procedural wrappers in for bc.
Then once core isn't using it, drupal_static() can be deprecated, then removed for 9.0.x.
Comment #10
Crell CreditAttribution: Crell at Palantir.net commentedWould it make sense to mark it deprecated now, slated for 9.x removal, so that contrib authors know to stop using it?
Comment #11
catchNo I think we need to only mark things @deprecated when there is active work to replace them with a definite alternative. A lot of things got marked deprecated in the 8.x cycle without a viable replacement, most infamously url() and l(), and that led to nasty surprises later on. So when there are no uses of drupal_static() in core, or if all the usages are only in functions that are marked @deprecated themselves, let's mark it, but not before.
Comment #12
almaudoh CreditAttribution: almaudoh commentedComment #13
xjmThis issue was marked as a beta target for the 8.0.x beta, but is not applicable as an 8.1.x beta target, so untagging.
Comment #19
claudiu.cristeaAdded #3035288: Deprecate theme_get_setting() as child issue.
Comment #20
claudiu.cristeaAdded #3035297: Don't use drupal_static in AutoIncrementingTestItem.
Comment #21
claudiu.cristeaTrying to revive this META. Fixing title, IS. Will open child issues.
Comment #22
claudiu.cristeaAdding new child issues.
Comment #23
claudiu.cristeaAdded #3035343: Deprecate drupal_attach_tabledrag(). Move its logic in Table form element, #3035345: Remove usage of drupal_static_reset() from ContentTranslationOperationsTest and identifying more.
Comment #24
claudiu.cristeaAdding #3035347: Remove usage of drupal_static_reset() from ContentTranslationTestBase, #3035349: Remove usage of drupal_static_reset() from EntityReferenceFieldTranslatedReferenceViewTest.
Comment #25
claudiu.cristeaComment #26
claudiu.cristeaAdded #3035352: [PP-1] Deprecate file_get_file_references(). Move the logic to file.usage service.
Comment #27
claudiu.cristeaAdding #3035361: Remove usages of drupal_static() & drupal_static_reset() from file_test.module.
Comment #28
claudiu.cristeaAdding #2536594: Add a FilterFormatRepository providing methods to load filter formats.
Comment #29
claudiu.cristeaAdded #3035376: Remove usage of drupal_static() from GetFilenameTest.
Comment #30
claudiu.cristeaAdding #2081585: [META] Modernize the History module.
Comment #31
claudiu.cristeaComment #32
claudiu.cristeaAdded #3035518: Convert ImageEffectsTest & ToolkitTest into Kernel tests.
Comment #33
claudiu.cristeaComment #34
Mile23New child: #3036010: Fold drupal_get_updaters() into Updater class, deprecate drupal_get_updaters()
Comment #35
andypostAnother static should gone in #2908886: Split off schema management out of schema.inc
Comment #36
claudiu.cristeaAdded #3036962: Remove usage of drupal_static() from install_profile_info().
Comment #37
claudiu.cristeaComment #38
claudiu.cristeaAdded #3036982: Deprecate locale_translation_get_projects() and locale_translation_clear_cache_projects().
Comment #39
claudiu.cristeaAdded #2660338: locale_get_plural call in PluralTranslatableString is wrong.
Comment #40
claudiu.cristeaComment #41
Mile23New child: #3037054: Deprecate drupal_static_reset() without @trigger_error()
Comment #42
claudiu.cristeaAdded #3037031: Convert locale.compare.inc to a service.
Comment #43
claudiu.cristeaAdded #3037156: Locale import history service. Deprecate locale_translation_get_file_history(), locale_translation_update_file_history(), locale_translation_file_history_delete().
Comment #44
claudiu.cristeaComment #45
claudiu.cristeaAdded #3037202: Remove drupal_static() from node_mark().
Comment #46
volkswagenchickComment #48
claudiu.cristeaAdded #3038908: Deprecate node_access_view_all_nodes(). Move its functionality in NodeAccessControlHandlerInterface.
Comment #49
claudiu.cristeaAdded #2171397: Move options_allowed_values() to a method somewhere ?.
Comment #50
claudiu.cristeaAdded #2044435: Convert pager.inc to a service.
Comment #51
claudiu.cristeaAdded #3038963: Remove the usage of drupal_static() from search_dirty().
Comment #52
claudiu.cristeaAdded #2083123: Shortcut cleanup: Remove duplicated code and deprecate legacy functions.
Comment #53
claudiu.cristeaAdded #3038971: Move system_get_module_admin_tasks() into a service and deprecate it.
Comment #54
claudiu.cristeaAdded #3039039: Deprecate some procedural functions in taxonomy.module.
Comment #55
claudiu.cristeaAdded #3039051: Remove useless static cache reset and route rebuilding from TaxonomyTranslationTestTrait(s) & TaxonomyTermViewTest.
Comment #56
claudiu.cristeaAdded #3039055: Remove useless reset of taxonomy_term_count_nodes static cache.
Comment #57
claudiu.cristeaAdded #3039074: Remove drupal_static() from _update_manager_(unique_identifier|extract_directory|cache_directory).
Comment #58
claudiu.cristeaAdded #2340341: Move template_preprocess, _template_preprocess_default_variables into services.
Comment #59
claudiu.cristeaAdded #3039081: Remove reset of user_role static cache in UserRoleAdminTest::testRoleWeightOrdering().
Comment #60
claudiu.cristeaAdded #3039248: Deprecate views_ui_contextual_links_suppress(), views_ui_contextual_links_suppress_push(), views_ui_contextual_links_suppress_pop().
Comment #61
claudiu.cristeaThe inventory is over. There are still few cases that are falling in the scope of #3037054: Deprecate drupal_static_reset() without @trigger_error().
Comment #62
claudiu.cristeaComment #63
claudiu.cristeaUpdating the list by moving the finished issues to a new section.
Comment #64
claudiu.cristeaWe should wait with deprecations until we have a decision in #3047289: Standardize how we implement in-memory caches.
Comment #65
markdorisonComment #66
claudiu.cristeaComment #68
daffie CreditAttribution: daffie commented#2044435: Convert pager.inc to a service moved to fixed.
Comment #69
daffie CreditAttribution: daffie commentedReplaced #3038963: Remove the usage of drupal_static() from search_dirty() with #3075703: Move search text processing to a service.
Comment #71
shaktikComment #72
shaktikComment #75
claudiu.cristeaComment #76
claudiu.cristeaUpdated the list.
Comment #77
claudiu.cristeaComment #78
claudiu.cristea#3039039: Deprecate some procedural functions in taxonomy.module is fixed.
Comment #79
claudiu.cristea#3035352: [PP-1] Deprecate file_get_file_references(). Move the logic to file.usage service is fixed.
Comment #80
claudiu.cristeaReverting wrong IS change.
Comment #81
claudiu.cristea#3037202: Remove drupal_static() from node_mark() is fixed.
Comment #82
claudiu.cristea#2412669: Remove drupal_static from BookManager is fixed.
Comment #85
samuel.mortensonI wanted to note that from reviewing a few related core/contrib isues like #2488458: remove calls to drupal_static(), I think we may be regressing on functionality that drupal_static() gave us.
I maintain Tome, a static site generator for Drupal, and one of the performance enhancements in that module is to perform multiple Drupal requests in one bootstrap (PHP process). Between requests, Tome performs a variety of operations to try to clear things in memory that were related to a previous request, and as a part of that calls drupal_static_reset(). See https://git.drupalcode.org/project/tome/-/blob/8.x-1.x/modules/tome_stat... for the full gory details. A goal of mine in the future is to contribute core patches for everything there that doesn't already use drupal_static() to use MemoryCache . For example I just found that \Drupal\views\Plugin\views\row\RssFields::render uses "static" where it should use drupal_static() or MemoryCache.
If core or contrib migrates usages of drupal_static() to storing things in protected variables or going back to using "static", it will make things a lot harder for me. Can we make sure that the guidance is to move to MemoryCache, not to come up with custom "cache in memory" implementations?
Comment #86
catchThat's being discussed in #3047289: Standardize how we implement in-memory caches, we might even want to postpone this on that issue?
Comment #88
catchComment #90
jweowu CreditAttribution: jweowu at Catalyst IT commentedI agree with waiting until there is a standard replacement for
drupal_static()
which provides equivalent functionality and can be easily employed to replace the original API calls.I'm really startled to observe that a lot of commits have been made replacing
drupal_static()
with plain PHPstatic
! To my mind that is counter-productive and achieves nothing more than the removal of useful functionality.I think those commits ought to be revisited once a new API is available so that they can be refitted with the new standard.
Comment #91
jweowu CreditAttribution: jweowu at Catalyst IT commentedPer #86, setting this to Postponed pending #3047289: Standardize how we implement in-memory caches.
(And once that is resolved, the previous changes made for this issue ought to be reviewed.)