@todo - write a nice explanation of the reasons (for now, @see comment #5) we should do this with examples on howto (for now, @see http://php.net/manual/en/language.namespaces.php, http://php.net/manual/en/language.namespaces.importing.php)
- #2093161: Remove all calls to drupal_json_encode() in favour of \Drupal\Component\Utility\Json::encode()
- #2093173: Remove all calls to drupal_json_decode(), use \Drupal\Component\Utility\Json::decode()
- #2089331: [meta] Replace calls to check_plain() with Drupal\Component\Utility\String::checkPlain()
- drupal_convert_to_utf8
- drupal_truncate_bytes (done!)
- truncate_utf8
- mime_header_encode
- mime_header_decode
- decode_entities
- drupal_strlen
- drupal_strtoupper
- drupal_strtolower
- drupal_ucfirst
- drupal_substr
- #2089433: Remove uses of deprecated XSS filter functions
- #2181507: Remove the deprecated usages of element_* methods
- #2393329: Replace all drupal_render calls with the service, and inject it, if possible.
- #2042165: Add a 'deprecated' module that includes deprecated functions only when enabled
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | deprecates-revealed-do-not-test.diff | 33.97 KB | Aki Tendo |
Comments
Comment #0.0
thedavidmeister commentedUpdated issue summary.
Comment #0.1
thedavidmeister commentedUpdated issue summary.
Comment #0.2
thedavidmeister commentedUpdated issue summary.
Comment #1
thedavidmeister commentedComment #2
thedavidmeister commentedComment #2.0
thedavidmeister commentedUpdated issue summary.
Comment #2.1
thedavidmeister commentedUpdated issue summary.
Comment #3
webchickCan we talk please about this a bit first, before jaunting off and creating 5000 sub-issues? :)
My worry with this is it pushes the concept of namespaces down to the .module file level. Modules are going to end up with a pile of "use" statements at the top just to get even the simplest thing done.
Would another approach be moving these types of things to the "Drupal" class so they're always available?
Comment #4
thedavidmeister commentedI will do no jaunting prior to discussion.
Comment #4.0
thedavidmeister commentedUpdated issue summary.
Comment #5
thedavidmeister commented@webchick - Wherever I see people knocking back patches because they use procedural "one line wrappers", I'm going to refer them to this issue to open up discussion.
Let me know if you disagree, but as I see it, not converting existing calls in core while simultaneously knocking back patches that are otherwise RTBC is counter-productive, so I'd like to raise the visibility of this discussion on those threads.
I don't intend for this issue to suggest deprecating functions that we still consider useful simply because the function is small - but I totally accept that with zero issue summary it's completely unclear what the intention here is at all (@todo!!!). The idea was to find functions that we already are considering "deprecated" in our patch reviews and explicitly document that they're deprecated; if we're OK with knocking back patches for calling a given function, let's give fair warning!
That said, I believe that functions that are *already* deprecated (documented as @deprecated) should be not be called by core and those conversions should go ahead (the drupal_json_X() calls for example). That is to say, I believe the immediate DX concerns of using the replacement methods are secondary to ensuring Drupal Core correctly uses its own API.
Procedural functions that exist purely to provide backwards compatibility should be converted too - we don't need the overhead of calling the wrapper. Arguably this criteria, by definition, means we should also deprecate these functions when we're no longer using them internally.
I've updated the issue summary to be more specific.
I do agree that having 5-20+ "use" statements in every file is tedious, but I also think that there are benefits (including performance) to calling methods directly instead of through "one line wrappers". I don't have any strong opinion on, or any kind of answer to which approach is "better". I presume "randomly use either" is likely the worst option.
I don't have any suggestions as to how we can improve the situation, TBH I don't know that I have the experience/knowledge to suggest something others wouldn't rightfully hate. I am skeptical that bulk moving methods to the "Drupal" class based on the criteria "we have a procedural function wrapping it for historical reasons" (if that is what you meant by "these types of things") would make sense - I doubt that's what you meant, but someone else coming to this thread might not realise, so we should try to outline what "these types of things" are.
@webchick, I'd love to chat about this and other issues in IRC with you, I'm sure I'd learn a lot, but your timezones are really bad for me so I often miss you by a few hours :( apologies for the perceived jaunting!
Comment #5.0
thedavidmeister commentedUpdated issue summary.
Comment #5.1
thedavidmeister commentedUpdated issue summary.
Comment #6
thedavidmeister commented@webchick, do you have any further comments on #5? I noticed that some of the sub-issues are being committed and I would actually be keen to open more cleanup issues, but I'm holding back based on what you said in #3.
Comment #7
sunComment #8
thedavidmeister commentedYay, more deprecatedness #2109793: Convert element_* methods in common.inc to a class
Comment #9
thedavidmeister commentedComment #10
thedavidmeister commentedComment #11
grom358 commentedhttps://drupal.org/comment/8582935#comment-8582935
Comment #12
jmarkel commentedDoing a little weeding post-NYCCamp sprint - removed Novice tag
Comment #13
thedavidmeister commentedComment #14
mile23Added this child issue: #2427637: Remove usages of deprecated entity_get_bundles()
Comment #15
Aki Tendo commentedThe attached patch adds "trigger_error('deprecation time: alternative function', E_USER_DEPRECATED)" to all deprecated functions, alters htaccess to not report those errors until the handlers are in place, DrupalKernel to return to E_ALL mode after setting up the handlers, and the handler itself to make just display the deprecated notices.
As should be safe to say - the patch returns a **lot** of hits, so I've labelled it do_not_test. We can consider the work on this ticket done when Drupal core can run this patch with E_ALL on at all times.
Comment #16
cilefen commentedComment #17
mile23There's a lot of overlap here: #2205673: [META] Remove all @deprecated functions marked "remove before 8.0"
Comment #18
mile23Old issue, new child here: #2311219: Fix hook_user_format_name_alter() documentation and stop referring to user_format_name()
Comment #25
mile23Adding related: #2959269: [meta] Core should not trigger deprecated code except in tests and during updates
Not all @deprecated code calls @trigger_error(), and finding the technical debt could be harder in those cases.
Also, not all items that should be @deprecated before Drupal 9 are marked as such. For instance #2748967: Trigger E_USER_DEPRECATED for BC support in simpletest_run_tests() as part of #2866082: [Plan] Roadmap for Simpletest I'm sure there are other less obvious examples.
Comment #26
gábor hojtsyComment #27
Aki Tendo commentedWell I did write that patch some time ago, but if I recall correctly it didn't take a long time to write - I simply did a search for @deprecated in sublime text and then added the trigger_error call where needed to cause the report. Building a new version of the patch (the old one is so out of date it probably can't apply) shouldn't take more than a couple hours.
Comment #28
volegerComment #29
andypostComment #30
berdirI'm closing this as a duplicate of #2959269: [meta] Core should not trigger deprecated code except in tests and during updates, all the specifically mentioned functions have been dealt with or actually their replacements already been deprecated and replaced again ;) And it seems to be about the same as the one referenced.