The D7 page callback for the path 'admin/config/content/scheduler/timecheck' needs to be converted to a route controller.

Change record: All functionality of hook_menu() is replaced by new systems for routing, menu links, local tasks, actions and contextual links.

This is part of #2426627: [meta] Convert all page callbacks to routes.

CommentFileSizeAuthor
#18 interdiff.txt3.86 KBpfrenssen
#16 scheduler_timecheck-2432345-16.patch7.27 KBpfrenssen
#9 timecheck.png29.4 KBpfrenssen
#5 scheduler_timecheck-2432345-5.patch7.5 KBherved
#3 convert-scheduler-timecheck-route-controller-2432345-3.patch9.2 KBbatigolix
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

herved’s picture

herved CreditAttribution: herved commented
Assigned: Unassigned » herved
batigolix’s picture

batigolix
Primary language Dutch
Location Utrecht
CreditAttribution: batigolix commented
Assigned: herved » batigolix
batigolix’s picture

batigolix
Primary language Dutch
Location Utrecht
CreditAttribution: batigolix commented
Status: Active » Needs review
FileSize
convert-scheduler-timecheck-route-controller-2432345-3.patch9.2 KB

Attached a first, rough & rocky patch.

It needs plenty of love & attention, but the 1st main step, adding the route controller, is done

The D7 code used a theme function, which seems a bit useless.
By removing it, I could skip learning how theme functions work in D8 (if they still exist)

batigolix’s picture

batigolix
Primary language Dutch
Location Utrecht
CreditAttribution: batigolix commented
Assigned: batigolix » Unassigned
herved’s picture

herved CreditAttribution: herved at Randstad Digital commented
FileSize
scheduler_timecheck-2432345-5.patch7.5 KB

Hello,

Thanks @batigolix for your patch.

But I was wondering wouldn't it be more convenient to output this information in the status page?
This way we rely on the "access administration pages" permission + we also get rid of the menu item and the controller.

I'm attaching a patch. Most of my work is based on the system module.
If I'm mistaken and a route controller is preferable then we can always combine both patches.

PS: Not really in the scope but I also removed the following unused "use" statement in scheduler.admin.inc: use Drupal\Core\Session\AccountInterface;

Status: Needs review » Needs work

The last submitted patch, 5: scheduler_timecheck-2432345-5.patch, failed testing.

herved’s picture

herved CreditAttribution: herved at Randstad Digital commented
Status: Needs work » Needs review

My patch should be fine however I believe the tests are broken ;)
We should look into #2418543: Relocate automated tests in PSR-4 namespaces asap.

PHP Fatal error: Class 'SchedulerApiTestCase' not found in /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh on line 649

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented

Yes the tests are currently broken. They still need to be ported.

I quite like the idea of moving this information to the "Reports" page, but I would like to have the opinion of @jonathan1055 on this as well.

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented
FileSize
timecheck.png29.4 KB

Here is how it looks in the status reports page:

herved’s picture

herved CreditAttribution: herved at Randstad Digital commented
Status: Needs review » Needs work

I found an issue in the code I copied from the system module. See #2296929: Remove system_requirements() SafeMarkup::set() use.
I mark this issue as needs work and I'll fix my patch as soon as it's resolved.

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented
Status: Needs work » Postponed

Postponing on #2296929: Remove system_requirements() SafeMarkup::set() use.

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented
Status: Postponed » Needs work

We actually don't need to wait on #2296929: Remove system_requirements() SafeMarkup::set() use to be able to complete this.

herved’s picture

herved CreditAttribution: herved at Randstad Digital commented
Status: Needs work » Needs review

Thanks pfrenssen,

So if #2296929: Remove system_requirements() SafeMarkup::set() use is not a drupal issue but a drush issue I move this ticket back to needs review.

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

The patch doesn't apply any more.

The last submitted patch, 3: convert-scheduler-timecheck-route-controller-2432345-3.patch, failed testing.

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented
Issue tags: -Needs reroll
FileSize
scheduler_timecheck-2432345-16.patch7.27 KB

Straight reroll.

  • pfrenssen committed 8945acd on 8.x-1.x authored by herved 
    Issue #2432345 by herved, batigolix, pfrenssen: Convert the timecheck...
pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented
Title: Convert 'admin/config/content/scheduler/timecheck' to a route controller » Convert the timecheck page to a status report
Status: Needs work » Fixed
FileSize
interdiff.txt3.86 KB

Looking good, made some final cleanups and committed to 8.x-1.x.

Changes made:

  1. Used shorthand array syntax
  2. The server time was mentioned twice, in the title and the first line.
  3. Removed referral link to Google, that's a bit condescending, people surely know how to use search engines.
  4. Removed mention of GMT, this has been superseded by UTC, the new standard.
  5. Store multiple lines of information in an array rather than concatenating them all together with <br> tags.

Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.