When all hook_menu implementations were removed, internal helper functions _tracker_myrecent_access()
and _tracker_user_access()
became unnecessary.
The first, _tracker_myrecent_access()
was replaced by declarations in tracker.router.yml
(tracker.users_recent_content
), tracker.services.yml
(access_check.tracker.view_own
), and src/Access/ViewOwnTrackerAccessCheck.php
(class ViewOwnTrackerAccessCheck
).
While _tracker_user_access()
was replaced by a declaration in tracker.router.yml
(tracker.user_tab
).
Nothing in D8 invokes these functions. There is a stale reference to _tracker_myrecent_access
in modules/migrate_drupal/src/Tests/Table/d7/MenuRouter.php
.
Comment | File | Size | Author |
---|---|---|---|
#43 | interdiff-41-43.txt | 1.31 KB | gaurav.kapoor |
#43 | unused-function-deprecated-2550581-43.patch | 2.37 KB | gaurav.kapoor |
#41 | unused-function-deprecated-2550581-41.patch | 2.44 KB | harsha012 |
#38 | unused-function-deprecated-2550581-38.patch | 2.43 KB | harsha012 |
#32 | unused-function-deprecated-2550581-32.patch | 2.39 KB | harsha012 |
|
Comments
Comment #2
Jeremy CreditAttribution: Jeremy at Tag1 Consulting commentedAttached patch removes both unused internal helper functions. No CR seems necessary as these were only internal helper functions (starting with
_tracker
) never exposed to other modules as an API.Comment #3
Jeremy CreditAttribution: Jeremy at Tag1 Consulting commentedUpdated patch to also remove a stale reference to
@see tracker_menu
fromtracker.pages.inc
. That function,tracker_page()
, is still invoked by the tracker module in multiple places (fromsrc/Controller/TrackerPage.php
,src/Controller/TrackerUserRecent.php
andsrc/Controller/TrackerUserTab.php
).Comment #4
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedAnd two to _tracker_user_access. Shouldn't we fix those while we are at it?
Comment #5
Jeremy CreditAttribution: Jeremy at Tag1 Consulting commentedThis patch isn't modifying the database dumps in
migrate_drupal/src/Tests
, only active code. The function_tracker_user_access()
doesn't show up in the D8 tracker module, so nothing to add here.Comment #9
Mile23Moving to 8.4.x because removing functions might be disruptive.
Patch no longer applies.
I see that migrate_drupal/tests/fixtures/drupal7.php has a reference to these functions, but that could be a reference to the D7 implementations. I'm not familiar enough with migrations to know for sure.
Comment #10
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-rolled.
Comment #11
Mile23Looks good. Thanks.
Comment #12
Wim LeersWoah, nice catch!
Comment #13
webchickLikewise with #2551259-11: Deprecate dead code locale_translation_manual_status(), we'll want to deprecate here rather than full-out remove.
Comment #14
Mile23Deprecation how-to: https://www.drupal.org/core/deprecation
Comment #15
Mile23Comment #16
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedAs per #15 added the @deprecated . please validate.
Comment #17
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedComment #18
Wim LeersComment #20
Wim LeersTestbot infrastructure failure:
Comment #21
xjmSo the difference between this and #2551259: Deprecate dead code locale_translation_manual_status() is that in the fine print of https://www.drupal.org/core/d8-bc-policy#underscore, underscore-prefixed functions etc. are explicitly internal. However, @catch and I discussed this and agreed that it's still probably best to deprecate things by default as a best practice, which is what the latest patch does, so +1.
Let's also explicitly add an
@internal
to these along with the deprecation. Then we can tack a message to the end of the standard deprecation message like:As internal API, this may also be removed in a minor release if it becomes needed.
Comment #22
xjmForgot to mention, we'll also update the deprecation documentation to cover this, so that we can deprecate internal API first since it's just less work, but still make it clear that it's internal API and so if there is a problem with it, we're not going to do refactoring to protect it.
Comment #23
catchMarking CNW for the additional @internal documentation.
Comment #24
xjmSorry, forgot to mark it NW.
Comment #25
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedAs per #23. Added @internal documentation. please validate.
Comment #26
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedComment #27
xjmUpdated the policy: https://www.drupal.org/core/deprecation#internal
Thanks @harsha012 -- looks pretty good. I would move the sentence from the
@internal
to the end of the@deprecated
. The example at the link above (which we just wrote now) should help. :)Also, be sure your comments wrap at 80 characters to follow our coding standards.
Thanks for helping with this!
Comment #28
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedAs per #27 made some improvement. please validate the patch
Comment #29
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedComment #30
xjmThanks @harsha012. Just a few more small issues.
. See https://www.drupal.org/core/deprecation for examples.
@trigger_error
? Is it excluded on purpose? See: https://www.drupal.org/core/deprecationcore/modules/system/src/Tests/Cache/GenericCacheBackendUnitTestBase.php
for an example. (I just picked that at random out of a grep.)Please take a close look at the examples in https://www.drupal.org/core/deprecation and try to update the patch based on that. Thanks!
Comment #31
xjmComment #32
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commented@xjm, please validate the patch now.
Comment #34
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedComment #35
xjmThanks @harsha012, that looks much closer.
See https://www.drupal.org/node/2550581
from the messages?Missing space between
_tracker_user_access()
andmay
.Ah, functional code does not need to be wrapped to 80 characters -- just the documentation. You can put function calls on multiple lines to improve readability, but we would do that by putting each parameter on one line -- but in this case, I think it's fine to just put this all on one line.
This line is just one character too long. :) Looks like you noticed that for the other copy of the docblock and rewrapped it (thence the missing space) but it didn't make it to this one.
We do still need to remove the other two references to this. (Either that, or remove this hunk from the patch, but it makes sense as part of this issue's scope since the functions we're deprecating were the helpers for it.) So there should be three lines like this in the patch for each of these references:
Comment #36
xjm(Sorry, I keep failing to actually mark the issue NW.) :)
Comment #37
xjmI updated https://www.drupal.org/node/2856615/revisions/view/10512193/10514226 to clarify that internal API changes might not have change records, including changing the example. Sorry for the confusion over that; this issue is helping us finalize the policy for these sorts of changes. :)
Comment #38
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commented@xjm, I hope now the issue will be fixed with this patch. please validate
Comment #39
Mile23Getting closer... Still doesn't address #35.1, though:
Comment #40
xjmThanks @Mile23.
These lines are also one character too long again. Please configure your editor or IDE to show you where 80 characters is so that you can wrap comments correctly. You can also still use Dreditor for now to see your patch on Drupal.org and review it. The documentation standards for this are here: https://www.drupal.org/node/1354#drupal
Comment #41
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commented@Mile23 and @xjm,
please check the below link for change recored. please validate it
https://www.drupal.org/node/2885190
Also made changes as per the comment #40.
Comment #42
Mile23Unpublished the change record since this hasn't been committed yet.
We still need to remove the links to this issue as per #39 and #35.1.
Comment #43
gaurav.kapoor CreditAttribution: gaurav.kapoor at OpenSense Labs commentedFixed 39 and 35.1.
Comment #44
Mile23Thanks! LGTM.
Comment #46
catchCommitted 950362e and pushed to 8.4.x. Thanks!
Comment #48
quietone CreditAttribution: quietone at PreviousNext commentedPublish the cr