- Issue created by @ericgsmith
- 🇳🇿New Zealand ericgsmith
Uploading example
views.view.content.yml
file with a json export attached to the default content view which can be used for testing. - 🇳🇿New Zealand ericgsmith
Another option would be to add an additional option such as in https://www.drupal.org/project/drupal/issues/2719797 ✨ New option for Views page displays to use the admin theme Fixed / https://git.drupalcode.org/project/drupal/-/commit/4dbf3f46c19766a38c3e4... to mark the path as an admin route directly in views.
I think instance, if the path is /admin it is showing "Yes (admin path)" - so perhaps instead of changing the formats on the route, the plugin can just mark the route as admin directly if the path starts with admin?
- Status changed to Needs review
over 1 year ago 2:00am 26 June 2023 - Open on Drupal.org →Core: 9.5.x + Environment: PHP 8.0 & MySQL 5.7
41:14 40:42 Queued - 🇳🇿New Zealand ericgsmith
Patch going with the option of marking the route as an admin route.
Setting as needs review to get feedback on the approach - we should be able to add a unit test to test the route collection.
- last update
over 1 year ago 4 pass - 🇳🇿New Zealand ericgsmith
Went for a kernel test so that the test can still be valid if the implementation differs from the proposed approach.
Interdiff is the test only patch.
- last update
over 1 year ago 3 pass, 1 fail The last submitted patch, 7: views-data-export-3368855-7-mark-as-admin-route-test-only.patch, failed testing. View results →
- Status changed to RTBC
over 1 year ago 12:03am 27 June 2023 - 🇳🇿New Zealand RoSk0 Wellington
Thanks Eric!
Looks good to me - approach is sensible, and fixes our unexpected behaviour with front theme on admin path.
- last update
over 1 year ago 3 pass, 1 fail - 🇭🇺Hungary mibfire
Actually the "batch" path should always use the admin theme regardless of views path starts with "/admin" or not, because this is also the case in the core:
system.batch_page.json: path: '/batch' defaults: _controller: '\Drupal\system\Controller\BatchController::batchPage' requirements: _access: 'TRUE' _format: 'json' options: _admin_route: TRUE
So here the batch uses always the admin theme and this should be the case with the patch for this issue. I removed the
str_starts_with($this->getOption('path') ?? '', 'admin/')
part from the patch.
The last submitted patch, 12: views-data-export-3368855-12-mark-as-admin-route.patch, failed testing. View results →
- Status changed to Needs review
over 1 year ago 12:15am 18 July 2023 - 🇳🇿New Zealand ericgsmith
Actually the "batch" path should always use the admin theme regardless of views path starts with "/admin" or not, because this is also the case in the core:
This is not true and is not the case in core.
The batch negotiator (
theme.negotiator.system.batch
) runs at a higher priority that the admin theme negotiator (theme.negotiator.admin_theme
)See https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/syste...
- 🇭🇺Hungary mibfire
@ericgsmith
Indeed, you are right. Thanks! I hid my patch.
- last update
about 1 year ago 4 pass - last update
about 1 year ago 3 pass - last update
about 1 year ago 3 pass - 🇳🇿New Zealand ericgsmith
Can you please explain your patch @erom?
If you are making changes to a patch you should supply interdiff and comment on the changes or its confusing to follow.
I've only glanced at it but it looks like the patch in 7 but without the test?
- 🇵🇭Philippines erom
@ericgsmith, my patch will just automatically use the admin theme when it's using the batch. it is not logically correct since some use cases should allow batch to appear within a theme, i will hide my patch..
- 🇵🇱Poland besek
I've tested patch #17 on Drupal 10.2.6 with Views Data Export 8.x-1.4 and it works really nice, thanks @erom.