Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I think that it would be great to have a page listing all available layouts. That's what I did in the attached Layout PLugin UI module. Could it be part of the Layout Plugin module?
Comment | File | Size | Author |
---|---|---|---|
#54 | interdiff.txt | 2.52 KB | willzyx |
#54 | add_page_listing_the-2786541-54.patch | 2.98 KB | willzyx |
#52 | add_page_listing_the-2786541-52.patch | 600 bytes | willzyx |
#45 | devel-2786541-45.patch | 9.68 KB | Manuel Garcia |
| |||
#45 | interdiff.txt | 1.27 KB | Manuel Garcia |
Comments
Comment #2
andypostPlease provide patch to apply to current code base, it will be easy to test/review
PS https://www.drupal.org/patch/submit
Comment #3
Manuel Garcia CreditAttribution: Manuel Garcia at Appnovation commentedHere's that exact same code in the form of a patch, in the hope of getting the ball rolling =)
Comment #4
andypostI think that basically could be just a build array, no reason for special theme function
Comment #5
romainj CreditAttribution: romainj as a volunteer commentedAgree. The idea was to be able to render the list in a nice way for example using a jQuery plugin. But it could be for later on.
Comment #6
DamienMcKennaHow about just making this a 'report' page?
Comment #7
dsnopekHm. layout_plugin is in core now, but this will be introducing something that isn't in core. Does this make sense?
Comment #8
DamienMcKennaIf not we might want to move it to e.g. the Devel module as a central place for developer-related things.
Comment #9
romainj CreditAttribution: romainj as a volunteer commentedThe Layout Plugin UI module was intended first to be a development module for themer. So I agree it could belong to the Devel module suite.
Comment #10
DamienMcKennaMoving to the Devel module.
Comment #11
moshe weitzman CreditAttribution: moshe weitzman commentedFor starters, any new submodule in Devel needs a maintainer who commits to managing the issue queue for at least a year. Please volunteer here.
Comment #12
romainj CreditAttribution: romainj as a volunteer commentedI volunteer for the Layout Plugin UI module.
Comment #13
romainj CreditAttribution: romainj as a volunteer commentedHere is the module.
Comment #14
willzyx CreditAttribution: willzyx commentedHi guys, thanks for contributing!
If we want really add this functionality to devel I see no reason for creating a new module in the devel suite.. we could simply create a controller and register its route using
_module_dependencies
. I'm missing something?also, since #2296423: Implement layout plugin type in core why we should integrate the layout plugin contrib module and not the layout discovery module shipped with core?
Comment #15
romainj CreditAttribution: romainj as a volunteer commentedI refactor the whole module in order to comply with the latest Drupal core version. Patch attached.
Comment #16
romainj CreditAttribution: romainj as a volunteer commentedComment #17
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedQuick drive by review, patch applies fine, enabled it and checked the permission page which has a problem:
Typo here
registred
->registered
Also I think the layout page itself looks empty of information on several columns, perhaps there are other bugs I didn't dig into...
Comment #18
romainj CreditAttribution: romainj as a volunteer commentedSorry for the typos :)
As for the empty columns it requires to provide the corresponding infos in the MODULE_NAME.layouts.yml file as follow:
I tried to use the
getRegionNames()
method without success.I will investigate further...
Comment #19
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedThe attached gets the regions' labels in place.
Core layouts have no description apparently, or at least they come as NULL, and the same goes for the icons paths.
Comment #20
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedI'm thinking it'd be also be useful to add a new column showing what provides each layout (
$layout->getProvider()
).Comment #21
andypost+1 to #20
Comment #22
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedTackling the typos on the permission, and adding the provider column.
Comment #23
romainj CreditAttribution: romainj as a volunteer commentedI think I'm late :) Anyway here my patch.
Comment #24
willzyx CreditAttribution: willzyx commentedsomeone can answer to #14? :)
Comment #25
andypostImo that should be just a controller and /admin/reports/... route
NW for early
render()
should be `drupal:layout_discovery`
it looks exactly like "report"
early render??
Comment #26
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedThanks guys for the reviews, I think #14 makes sense btw. Working on this today.
Comment #27
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedOK here is a different path, getting this into devel itself.
_module_dependencies: 'layout_discovery'
Comment #28
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedPls ignore patch
devel-2786541-26.patch
:)Comment #29
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedAbout the layout icons... see #2660124: Dynamically build layout icons based on well formed config
Comment #30
andypostNice, imo rtbc but I'd like to see todo in code pointing to related
Comment #32
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedGood call @andypost - adding the @todo and fixing the comment on
LayoutInfoController::layoutInfoPage()
.Comment #33
willzyx CreditAttribution: willzyx commentedAnd propably we need a basic test coverage for this new functionality! ;)
Comment #34
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commented@willzyx you're right of course :)
Here's #32 plus the test checking that the page is accessible, that the title is there and the column names as well.
Not sure we should be checking the contents of the table itself, since layout_discovery should be testing its layotus anyway...
Comment #35
andypostThanx! Imo good to go
Comment #36
romainj CreditAttribution: romainj as a volunteer commentedDevel module does not work anymore if you do not install the Layout Discovery module as well. We do have a dependency to the Layout Discovery module in the routing file but not in the link pointing to that route. Is that possible to declare a dependency in the devel.links.menu.yml file?
Comment #37
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedThanks @romainj you're absolutely right, having a look at this...
Comment #38
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedOK, we cant use
_module_dependencies
on the menu yml file, but we can make use ofhook_menu_links_discovered_alter()
for this purpose. Attached patch does just that. Also it'd be great if core took care of this for us :)Comment #39
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedAs a side note, I have added tests for the other pages in the hope that those would prevent us nearly breaking all things in the future like with this patch... have a look #2876073: Expand test coverage for page controllers :)
Comment #40
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedEnds up devel already has tests that cover those pages, so not sure why the patches before #38 were coming back green... anyway, we should be in the clear now.
Comment #41
willzyx CreditAttribution: willzyx commented@Manuel Garcia great work. A quick review:
Can we import TranslatableMarkup class instead of using FQNS?
Now that the functionality is integrated in the devel module can the menu link be placed in devel menu? also is there a strong motivation to expose it in report section?
Missing constructor docblock. @inheritdoc is not applicable here :p
can we move the test to BTB?
I think that It might be worth to expalnd a little the test whit some basic checks like:
- is the page accesible for unprivileged user?
- is the menu link hidden when layout_discovery is not enabled and visible otherwise?
- what appen when layout_discovery is not enabled? devel works properly?
You can find some inspiration in the other devel functional tests here ;)
Comment #42
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedThanks @willzyx for the review! A little progress on that:
Still to do: #41.4
Comment #44
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedDoing #41.4 and removing the old test.
I did expand on what we had before, so on top of what we had we now also check that:
Scenarios not tested yet:
I have also added a small paragraph with help text for the page.
I thought about adding the search like on the other info pages, but not sure its necessary on this case.. will there be loads of layouts on sites? We could hold this back until people request it perhaps.
Comment #45
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedAdding tests for when the layout_discovery module is not installed.
Comment #49
willzyx CreditAttribution: willzyx commentedCommitted and pushed to 8.x. Thanks @romainj @Manuel Garcia and all the others!
Comment #50
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedBrilliant, thanks!!
Comment #51
willzyx CreditAttribution: willzyx commentedSeems that DevelLayoutInfoTest cause test failures in D 8.1 and 8.2 due to missing dependency layout_discovery :(
https://www.drupal.org/pift-ci-job/671024
https://www.drupal.org/pift-ci-job/671025
Comment #52
willzyx CreditAttribution: willzyx commentedI'm not aware of a clean solution for solving the tests failure.. Something like the attached patch could works
Any help/suggestion is really appreciated :)
Comment #53
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedOh wow, did not think of that!
Patch looks like it makes sense, though I haven't seen anything else in the wild trying to do this...
Comment #54
willzyx CreditAttribution: willzyx commentedok I haven't found clean solution for solving the tests failure :( .. for now the solution in #52 could works. Added a todo for this
the attached patch also contains some minor fix to tests
Comment #56
willzyx CreditAttribution: willzyx commented