Problem/Motivation
As raised on #1938062-54: Convert the recent_comments block to a view it might be really useful to provide additional dropdown links
on the block overview page.
Some examples could be an edit link for a menu, and edit link for the view, the aggregator feed configuration etc.
Proposed resolution
Add a new method to all block plugin using deriver to add extra operation links.
For menu_ui and views_ui use hook_entity_operation().
Remaining tasks
Update patch, use the one in #183 or #196.
Add test
Add before and after screen shots to the Issue Summary
Add change record
Review
Commit
User interface changes
Before
After
API changes
Added new \Drupal\Core\Operations\OperationsProviderInterface
to add extra opertaion links.
Data model changes
None
Original report by @dawehner
As raised on #1938062-54: Convert the recent_comments block to a view it might be really useful to provide additional dropdown links
on the block overview page.
Some examples could be an edit link for a menu, and edit link for the view, the aggregator feed configuration etc.
Comment | File | Size | Author |
---|
Issue fork drupal-1956134
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
jibranTagging
Comment #2
damiankloip CreditAttribution: damiankloip commentedAt the moment this will be really difficult, as there is no easy/nice way to add to the operation links in list controllers. Me and Gabor were talking about this last week. I think this will be dependent on some sort of operations API?
Comment #3
dawehnerIt seems to be that operations API is way to complex, not sure. You could for sure always add a new method on the Block Interface that allows you to specify some links depending on your block.
Comment #4
damiankloip CreditAttribution: damiankloip commentedYeah, that would work ok. I guess that it would be nice if we had this for configuration entities in general.
Comment #5
dawehnerSo basically the entityInterface would get a new method which returns the links in a non-render version (something which can be utilized/tested easily).
Then the listing controller and views could ask the entity to return it's links. On top of that the block entity would ask the block plugin about it's links which then would know how to link to the menu edit link.
I vote to make it as easy as possible that we can shift towards the operations api later.
Comment #6
damiankloip CreditAttribution: damiankloip commentedYep, we talked about this is IRC, I think that is a good plan forward. We need to ask this on a per entity level, so we get the flexibility. So probably the getOperations() method on the controller can also ask the entity for it's links. The implementation across entities can get/add this data how they please.
Comment #7
dawehnerStarted with some basic and really simple code adding this to the entity and block plugin interface and using it for edit menu links.
Needs review more in like "needs more discussion".
Comment #9
dawehnerFor sure, views breaks this :)
Comment #10
xjmRelated (I think): #1956644: Add "edit" to dropbuttons on the block admin page.
Comment #11
xjmActually, marked that one as a duplicate of this.
Comment #12
xjmHere's what this patch does:
Two out-of-scope things I noticed:
Comment #13
xjmMy original suggestion in #1938062-54: Convert the recent_comments block to a view was for the block library page or the block instance editing form rather than the theme/display block instance admin listing, but we should probably consider all of the above and see which are the most helpful and non-confusing.
For custom blocks, I think we might want to make the link say "Edit block content".
Comment #14
dawehnerI agree that it should be actually on both places? I guess users want to save time, if possible.
Comment #15
Bojhan CreditAttribution: Bojhan commentedJust wondering, if there is overlap between instance and object settings? E.g. menu instance settings, and menu settings?
Comment #16
xjmSee also:
#1956504: Add block instance delete to block contextual links
#1956496: Custom block contextual links should differentiate between editing/deleting the block instance and the custom block entity
Comment #17
klonosI think that it is best not to expose the term "instance" the same as it is not a good idea to expose "plugin". How about we have "Edit block"/"Delete block" and "Edit block type"/"Delete block type". Code-wise we'll know that by "type" we actually refer to the respective plugin and UI-wise the non-"type" actions refer to the configurations.
Also, using the term "Delete" all over does not feel right. Perhaps we should use "Remove" when the actual thing is simply removed from where it was placed and reserve "Delete" for when we are actually deleting.
Comment #18
xjmAgreed about "instance". The problem though is that "block type" is already the bundle (like the content type) of custom blocks. Even if it weren't, it would be "Menu block," "Views block," "Content block," etc.
Comment #19
dawehnerI'm wondering whether we should add both test coverage for operation links in general + operations links on blocks + the available operation links on menu block?
Comment #20
xjmRelated: #889378: Create taxonomy fields from the taxonomy administration page
Comment #21
jibran#9: drupal-1956134-9.patch queued for re-testing.
Comment #23
dawehnerRerolled. Adding vdc tags just because we add vdc tags to pretty much everything ;)
Comment #24
dawehner#23: drupal-1956134-23.patch queued for re-testing.
Comment #25
jessebeach CreditAttribution: jessebeach commented+1 to this integration. I half-expected this existed already.
Comment #26
jibran#23: drupal-1956134-23.patch queued for re-testing.
Comment #28
elachlan CreditAttribution: elachlan commentedRerolling patch because of error.
Comment #29
dawehnerLet's provide an @inheritdoc and remove all this kind of whitespace/tabs ...
Comment #30
elachlan CreditAttribution: elachlan commentedRemoved the white spaces.
Not sure about providing @inheritdoc.
Also when I manually tested it, it seems like the functionality described in #12 did not work.
Comment #31
dawehnerWhy are you not sure about the {@inheritdoc}?
Comment #32
elachlan CreditAttribution: elachlan commentedI am new and don't know how to go about doing it.
Where does it need it?
Comment #33
jibranIf #1938062: Convert the recent_comments block to a view is block on this how come this is a feature request. Rerolled #23
@elachlan Thank you for contributing to drupal please see https://drupal.org/node/1354#inheritdoc for more info on inheritdoc.
Comment #34
dawehner#33: 1956134-33.patch queued for re-testing.
Comment #35
dawehner{@inheritdoc}
Comment #37
jibranReroll and fixed #35.
Comment #38
dawehnerThat allows us to provide better UX, so +1 (though i mostly wrote the code).
Comment #39
afeijo+1
it is very nice to have that handy Edit link there!
(I had to ccache and save the blocks page to reflect it)
Comment #40
jessebeach CreditAttribution: jessebeach commentedI applied the patch in #37 and cleared cache; saw no change. I reinstalled; saw no change. This is what I'm seeing for the recent comments View block.
I'm a little lost as to what I should do to see a change. Hint?
Comment #41
afeijo@jessebeach
did you try what I wrote? save the blocks admin page, no need to change anything
after I did that, the page noticed the new menu items
Comment #42
dawehnerComment #43
jessebeach CreditAttribution: jessebeach commented@afeijo, I just tried that and I saw no change. Is there some file I could put a breakpoint in to verify that the code is running correctly or not?
Comment #44
dawehnerThis time it even works.
Comment #45
afeijoYay, now I just need to apply the patch, hit F5, and the Operations buttons show the Edit menu link
I did it 3 times with git reset --hard, F5 (gone), git apply -v {patch file}, F5 (show)
Comment #46
jessebeach CreditAttribution: jessebeach commentedAlright! I'm getting an "Edit menu" option for my custom menu and the build-in menus.
"Edit menu" should be lowercase as "edit menu" to follow the pattern currently established in the list of operations.
I'm still not seeing an option to edit a view on the the "Recent comments" view block.
Comment #47
elachlan CreditAttribution: elachlan commentedChanged to lower case.
Comment #48
elachlan CreditAttribution: elachlan commentedComment #49
dawehnerI thought we decided somewhere to use uppercase for new links?
Comment #50
jessebeach CreditAttribution: jessebeach commentedPerhaps, but I'd rather keep consistent with the pattern that's there now, even if it's the wrong one. Being inconsistent looks worse than capitalized or uncapitalized.
Comment #51
Bojhan CreditAttribution: Bojhan commentedCapitalised is the new standard.
Comment #52
afeijoI'd love to do the patch to capitalize all labels
May I? In this issue or a new one?
Comment #53
elachlan CreditAttribution: elachlan commentedafeijo, just do it in this issue for the ones relevant to the menu we are working on.
Comment #54
afeijoHere it is, elachlan :)
Comment #55
elachlan CreditAttribution: elachlan commentedLooks good! Test passed.
Comment #56
elachlan CreditAttribution: elachlan commentedComment #57
alexpottNeeds a reroll...
Comment #58
elachlan CreditAttribution: elachlan commentedI believe the problems with BlockListController.php are caused by the changes done in #2027857: Blocks operations cannot be altered
It added this:
So I think we just have to change that instead.
Comment #59
elachlan CreditAttribution: elachlan commentedI don't know why the tags were removed.
Comment #60
dawehnerThere we go.
Comment #61
jibranBack to RTBC.
Comment #62
alexpottThis patch has a complete lack of tests.
We should not be using user_access here anymore. We probably will have to use
Drupal::request()
here but that's better than calling out touser_access()
.AFAICS there is no
getOperationLinks()
method on Drupal\views\Plugin\Core\Entity\ViewComment #63
dawehnerThe ViewUI class is a decorator around entites, so everytime you add new methods, you have to implement that. In general I would like to see some views related links on the View entity object.
Comment #64
jibran#60: block_links-1956134-60.patch queued for re-testing.
Comment #66
jibranNeeds reroll as per #65.
Comment #67
dawehnerjust a rerole, there are no tests yet.
Comment #69
garphy CreditAttribution: garphy commentedRerolled.
Comment #70
benjy CreditAttribution: benjy commentedNeeds a reroll
Comment #71
grisendo CreditAttribution: grisendo commentedRerolled
Comment #72
andypostcustom block have no 'edit/configure' link now, seeps this hunk could be added here
Comment #73
dawehnerYeah, let's open a follow up for that.
Comment #74
andypostFiled issue with patch #2139141: BlockAccessController::checkAccess() is not executed if any hook_block_access() returns data
Comment #75
star-szrTagging for reroll.
Comment #76
jbloomfield CreditAttribution: jbloomfield commentedAttempting a patch re-roll.
Comment #77
jbloomfield CreditAttribution: jbloomfield commentedRe-rolled patch.
First time so be nice :)
Comment #78
benjy CreditAttribution: benjy commentedremove white space.
Should be \Drupal::currentUser()->hasPermission()
Maybe we should add a @todo to replace once http://drupal.org/node/1874498 lands.
Comment #79
jbloomfield CreditAttribution: jbloomfield commentedHi benjy,
As I am new to this, is it up to me to fix your changes above or the original patch creator? I don't mind either way, I was just doing my first patch re-roll.
Cheers
John.
Comment #80
dawehnerThere is no rule at all, who has to fix things. If you can fix those, please do it. Thank you!
Comment #81
jbloomfield CreditAttribution: jbloomfield commentedWill do, thanks.
Patch Re-roll.
Comment #82
jbloomfield CreditAttribution: jbloomfield commentedRe-rolled the patch and also created an interdiff.
There are already @todo's throughout the file mentioned below.
Comment #83
jbloomfield CreditAttribution: jbloomfield commentedBenjy, sorry I have just spotted what you meant about the @todo.
In the
public function getOperationLinks()
it needs one. I will re-roll another patch later tonight.Comment #84
jbloomfield CreditAttribution: jbloomfield commentedWill re-roll another patch to add a @todo.
Comment #85
jbloomfield CreditAttribution: jbloomfield commentedRe-rolled patch again to add a @todo.
Comment #86
jbloomfield CreditAttribution: jbloomfield commentedRemoved "Needs reroll" tag
Comment #87
jbloomfield CreditAttribution: jbloomfield commentedOk, so I have tested this on a fresh D8 install and the "Edit Menu" link fails.
It goes to a path of "/admin/structure/menu/manage/admin/edit"
The actual path should be "/admin/structure/menu/manage/admin"
Will fix the "Edit Menu" link.
Comment #88
jbloomfield CreditAttribution: jbloomfield commentedFixed the "Edit Menu" path.
New patch and interdiff.
Comment #89
tim.plunkettThis seems related to #1839516: Introduce entity operation providers
Comment #90
dawehner88: block_links-1956134-88.patch queued for re-testing.
Comment #92
klonos#2152377: Failed to change the status to 'needs work' after test failure
Comment #93
dawehnerReroll.
Comment #96
larowlanrerolling
Comment #97
larowlanNot sure where the changes to Menu entity came in, removed them.
Added some tests.
Screenshot
Comment #98
dawehnerThank you so much for the reroll!
I wonder whether we should extract the operation links interface from blocks and entities into a generic one?
On the longrun we could think of using a link object but this would be inconsistent with the other operations for now.
Comment #99
larowlan98.1 fixed
98.2 agreed, started off using Url objects but then realised that list builders don't work that way.
Comment #100
Dave ReidSeems like extraneous implementer effort here to manually check access here. Could we automatically determine if the user has access to that link based on the route_name and route_parameters provided?
Comment #101
dawehnerWe can indeed and we should, see #2302563: Access check Url objects
Comment #102
larowlanPostponed on #2302563: Access check Url objects
Comment #103
XanoWhat exactly does a
BlockPluginInterface
do? What are the operations for? If the operations a block instance provides are specific to that instance, then this approach is okay. If the operations are not specific to that instance, but to all instances of that plugin, then we need to let plugins define an operations provider.In short: plugin instances should have a single task. It's not good if we instantiate plugins for different 'scopes'.
Comment #104
larowlanBlocker is in, needs reroll to remove access check
Comment #105
larowlanAddresses 100
Re 103: Yes specific to an instance - eg example here applies to specific instance of menu block derivative (SystemMenuBlock) - we have derivative and plugin id in scope.
Comment #107
larowlanMenu route name changed
Comment #109
larowlanComment #110
larowlanWrong patch
Comment #113
larowlanmenu ui module doesn't always exist
Comment #114
dawehnerI am fine with the current status, anyone up for a review?
Comment #115
tim.plunkettI can't tell if its just that this patch only does a couple conversions, or if this is all of it, but I'm hesitant to force this on each interface. Why not let the specific classes (or even just the base class) implement it, and add a simple instanceof check to the relevant calls?
Comment #116
larowlanBlock content will add one (see postponed ref issue)
Views will use it.
We have base implementations for both so there is no work for most plugins.
Comment #117
dawehner@tim.plunkett
Great idea!
Comment #118
larowlan+1 RTBC, thanks @dawehner
Comment #119
tim.plunkettThis can be removed now.
I think either this should be moved out of the Entity namespace, or the description should be much more specific about what it is used for.
Currently magicalponies.com is not registered (what an opportunity!) but I think it should be example.com or something just in case :)
Comment #120
larowlan@tim.plunkett
Comment #121
dawehnerThank you for the review!
Meh.
Comment #122
tim.plunkettPHPStorm--
Comment #123
dawehnermeh!!!! SO yeah, why does storm not solve all the problems!
Comment #124
Wim LeersThis very much reminds me of contextual links. So:
ContextualLinkManager
+*.links.contextual.yml
, rather than creating a similar (I'd say parallel) system?OperationsProviderInterface
?Translate
operation. Isn't that a problem?Code review
Just one stupid nitpick:
No need for a fully qualified class here.
Comment #125
dawehner@Wim Leers
Thank you for your review.
So you back to the almighty operations API which will just be impossible to solve. Note: This patch just exposes entity operations onto plugins.
Everyone would love to have one, which would allow you to configure which contextual links / entity operations / sibling local tasks you want to see, but yeah I don't think this is feasible at least in 8.0.x
For example noone came up with an idea how to support things like creation links.
storm--
Comment #126
Wim LeersBut that's not an operation on an existing thing, so … I don't see why that's needed?
I know that my question kind of leads to the "almighty operations API" discussion, but OTOH, we want to make sure we don't have two ways for doing the same thing. And this is even worse: it forces us to define the same things twice! Contextual links *are* for operations after all — I'd argue it should be renamed to Operation Links in D9.
So while I don't want to slow things down unnecessarily, I think it's essential to have a discussion here about contextual links. I don't understand how this issue could've come to comment 125 without having discussed contextual links at all? :/
Conclusion: Am I missing something, or why can't we reuse
*.contextual.links.yml
for these operation links?Comment #127
dawehner@Wim Leers
Maybe I don't 100% get your point.
Well, mostly because these links are used as part of the entity listing as well, admin/structure/block is the one you will look at. Do you really want to rewrite all of the operations
in core to support contextual links as part of this issue? On top of that contextual links are tight to a specific visual representation atm. which we will need to decouple in order to make this happen.
Comment #128
Wim LeersOh! I think I know see why you misunderstand me :)
I didn't mean
, I meant !I think the data in
*.links.contextual.yml
is directly reusable?Comment #129
dawehnerOh I see, well on an abstract level you certainly could. On a congrete level this would require a lot of work, as for example you would have to provide multiple groups: one for the entity type, and one for the entity type together with the specific block plugin.
Comment #130
Wim LeersBut the "multiple groups" thing is already the case for contextual links and works fine there, right?
Comment #131
olli CreditAttribution: olli commentedCouldn't find an issue to add an "Edit view" link so I filed #2346285: Provide 'Edit view' link on "admin/structure/block".
Comment #132
olli CreditAttribution: olli commentedSo, what is the way forward here? Is #125 the one to review?
Not sure how #126-#130 would work but adding an "Edit view" link on top of #125 in ViewsBlockBase was easy even for me =). Would it be possible to have a similar method that would automatically add contextual links for my block? Even without adding them in *.links.contextual.yml?
Comment #133
XanoOperationsProviderInterface
was already added elsewhere, but no code in core uses it yet. I opened #2398601: Improve OperationsProviderInterface to improve on what we have now.Comment #135
dawehner@olli
Yes, this is what we need at that given point in time. At the same time I would bet that we need a reroll as well.
Comment #137
dawehner.
Comment #138
olli CreditAttribution: olli commentedHere's a reroll.
Comment #139
olli CreditAttribution: olli commentedWould it be possible to load the menu and check if it has an edit-form link rather than checking moduleExists()? Also, shouldn't this check if user can access the url or edit the menu?
How about adding another method to test getOperationLinks?
Comment #140
mgiffordthis should be a straight re-roll.
Comment #141
RavindraSingh CreditAttribution: RavindraSingh commentedLine needs to break after if chars count > 80
Remove White Space
Comment #142
tim.plunkettDisregard the first part of #141. It's an 80 character limit but for comments.
Comment #143
RavindraSingh CreditAttribution: RavindraSingh commentedOh yes, it is 80. Corrected
Comment #144
tim.plunkettRegardless, we don't break lines of PHP no matter how long they are.
Comment #145
lokapujyaMoved the test to it's own method.
So, we don't need the viewsUI operation links in this patch that was removed in #117?
Comment #146
lokapujyaWrong patch. Redo.
Comment #147
lokapujyaand the interdiff.
Comment #148
dawehnerGreat work!. It is good to see, that this issue got momentum again.
I would love to see edit links to views but we can for sure do that later as well.
Comment #149
larowlanre-roll - I think this is rtbc now but have worked on it a few times, so disqualified
Comment #150
andypostPatch looks great, except one nit.
RTBC +1
extra line is not needed, also use
Block::create()
according #2225347: [META] Replace calls to ENTITY_TYPE_load() and ENTITY_TYPE_load_multiple() with static method callsComment #151
larowlanfixes #150
Comment #152
andypostThanx :)
Comment #153
Wim LeersSeeing this made me wonder: "how is this different than contextual links?".
… turns out I wondered the same 6 months ago. See #124 — #132.
Sorry, but I really think we should formulate a proper answer to that. I imagine a committer would ask that question too.
Comment #154
mgiffordWho is best to formulate a proper answer to "how is this different than contextual links?"
Just nudging this issue as it's been sitting here for 4 months. Can someone get assigned to look at this?
Comment #155
dawehnerWell, this is not only about your loved contextual links but also about links on the block UI.
Comment #158
MattA CreditAttribution: MattA commentedDidn't think so...
Comment #159
jgeryk CreditAttribution: jgeryk commentedGoing to reroll
Comment #160
jgeryk CreditAttribution: jgeryk commentedrerolled
Comment #161
MattA CreditAttribution: MattA commentedComment #162
EclipseGc CreditAttribution: EclipseGc commentedThis seems completely unnecessary to me. The block entity can have this method without needing some instanceof check. If we want to implement the interface explicitly on the entity, that's fine (or have BlockInterface extend it), but the ListBuilder certainly doesn't need to explicitly check. We know this is part of the contract.
The rest of this patch seems pretty straight forward and obvious. Having plugins implement the interface (and subsequent instanceof checks as appropriate) is super sensible. Likewise, to Wim's question, it seems to me as though the contextual links code should probably call this as well.
Eclipse
Comment #163
mgiffordOk, then we should be able to nix this too.
+use Drupal\Core\Operations\OperationsProviderInterface;
Comment #164
andypostnot needed
this needs CR
nit, needs description
The module handler.
Comment #165
EclipseGc CreditAttribution: EclipseGc commentedCan we get interdiffs? Makes subsequent reviews a LOT faster.
Eclipse
Comment #166
catchThis blocks #2331603: Where to edit the contents of a custom block is not discoverable so bumping priority.
Comment #167
lokapujyaProviding an interdiff.
Comment #168
EclipseGc CreditAttribution: EclipseGc commentedThis still needs to be done, but since it's on the entity directly, there's no need to check the instanceof check since this is a builder literally for that entity.
Honestly, there's a lot of extra code here, odds are in the BlockListBuilder, we could circumvent doing anything with the Entity at all (in terms of new interfaces and such) and instead do:
That would circumvent all the additional code for the block Entity itself and get straight to the job of checking the block plugin for operation links (which has to happen anyway).
I don't like all the additional code added to the Block plugin just to check if menu_ui is installed. Perhaps that's unavoidable, but it sucks.
Eclipse
Comment #170
yoroy CreditAttribution: yoroy commentedComment #172
dawehnerJust rerolled the patch and addressing the feedback of @andypost
Comment #173
andypostMaybe better to move that to menu_ui module?
Comment #174
tim.plunkettCan this be closed now that Settings Tray is in core?
Comment #175
Bojhan CreditAttribution: Bojhan as a volunteer and commentedI am not sure? Accessing these settings from the blocks page is still not that easy?
Comment #176
tim.plunkettWell instead of editing it from admin/structure/block, you would be able to edit it from the front end
Comment #178
jibranRe: #177: I still think we should keep this.
Re: #173: I think it is fine. If we move the links then we need a hook alter for this.
ack -l `find core/ -type f -path \*src/Plugin/Block\*` --match deriver
.\Drupal\views\Plugin\Block\ViewsBlockBase
and\Drupal\block_content\Plugin\Block\BlockContentBlock
Comment #180
jibranFixed the fails.
Comment #181
dawehnerShouldn't we apply access checking here?
Comment #182
jibranRe #181: is it needed? I'm not sure.
@dawehner told me to make my case whether this issue is still helpful or not.
I have never use outside_in or quickedit on a production site in D8. I was never a fan of IPE in D7 as well but I like that views blocks have the ability to override views settings. I even use ctools_views module which further enhances this functionality so for me, this is more in line with that.
Comment #183
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedJust tested the latest patch, I really like this!
One thing I'd like to add to it is that when you click on edit view, or edit custom block, after you save it, you don't go up back at the block layout page.
Attached my humble approach to doing this =)
Comment #185
tim.plunkettHad completely forgotten about this issue!
This setOptions([]) looks both pointless, but also probably important?
Can a comment be added to explain it?
nit: Most of our test users are named either "test user" or "foobar". Who's tony? :)
I personally hate moduleExists checks like this. Seems to always indicate an inflexible system. If core has to resort to this approach, how could core swap in a better one?
Could menu_ui and views_ui swap out or decorate the plugin themselves?
Comment #189
mlncn CreditAttribution: mlncn at Agaric for Drutopia, Find It Cambridge, Cambridge, Massachusetts Family Policy Council commentedUm.... bump?
I came looking to find or file an issue about putting an edit link on the configure block page—not on the block listing page like this issue, but the individual block configuration pages such as /admin/structure/block/manage/example
Anyone who worked on this issue know if there's an issue for that... and might this other approach be less likely to take a biblical number of years? I guess that depends on if adding a tab that takes one to another tab context—Edit has Delete and Manage display as sibling tabs, and now also Configure block, but pressing on Configure block takes away the Delete and Manage display tabs and adds anything that a contrib module adds, such as Automatic label, but also has the Edit tab that, again, switches the context of sibling tabs when pressed.
Anyhow, with that idea put out there as an addition (or if worst comes to worst an alternative) to this issue, let's see if this patch still applies...
Comment #191
Oscaner CreditAttribution: Oscaner at CI&T commentedFor core 8.7.x
But i don't know how to write test script, so need guys help me to implement the Test.
Comment #192
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer commentedHad a brief look at this, and thought I'd share a status update for anyone willing to push this forward.
The patch to work on is #183, which needs a reroll, however this happened since: #2647124: Remove unused OperationsProviderInterface
So this means the reroll needs a decision to be made about this, I see two options:
OperationsProviderInterface
Comment #193
Oscaner CreditAttribution: Oscaner at CI&T commentedComment #194
SKAUGHTComment #196
Oscaner CreditAttribution: Oscaner at CI&T commentedI need quick work for me on Drupal 8.8.x, so used
Restore OperationsProviderInterface
solution.Comment #201
3CWebDev CreditAttribution: 3CWebDev commentedPatch from #196 applied cleanly on Drupal 9.4.1 and added context links to Edit Block Content on the block over page. No issues found. What are the steps to get this functionality in core?
Comment #202
rootworkI'm guessing this needs some submaintainer reviews given the length of time since it was last closely looked at (despite the fact that it still applies).
However not knowing which might be required, I'm going to set this as RTBC, given that it does apply, to see if that gets it more attention.
Comment #203
quietone CreditAttribution: quietone at PreviousNext commented@rootwork, to be committed a patch much pass the core gates. The contributor guide on Drupal.org has information about the process to Review a patch or merge request.
This is a bit confusing. The patch in #183 was reviewed in #185. Yet the patches from #191 to #196 change different files and do not include the tests from the earlier patch. I don't know enough about this to say which is preferred. Someone familiar will need to check the patch in #183 and #196 to decide which to use. Setting to NW
This is tagged for a change record, that needs to be added
Comment #204
smustgrave CreditAttribution: smustgrave at Mobomo commentedReading #203 decided to start at #183
Left a comment for #185.1
Updated #185.2 but there are other instances of tony in core just fyi.
#185.3 I removed the moduleExists for menu_ui but left it for views_ui. I see instances where it was currently used in views core src folder. Also there could be scenario where views_ui is disabled so would make sense to not have an edit link.
Added back OperationsProviderInterface even though it was removed in https://www.drupal.org/project/drupal/issues/2647124. Appears to have a purpose now.
Uploaded before/after screenshots.
Uploaded a tests-only patch based on the tests previously there.
@quietone what kind of change record should be made? For the new interface?
Leaving in NR to address change record and to see if tests pass.
Comment #205
smustgrave CreditAttribution: smustgrave at Mobomo commentedRemoving blocker as that ticket was closed as duplicate.
Comment #206
smustgrave CreditAttribution: smustgrave at Mobomo commentedNoticing failures in #204 test run.
Adding moduleExists back to menu_ui seems to fix some of them.
Comment #207
smustgrave CreditAttribution: smustgrave at Mobomo commentedCleaned up the tests some.
Fixed the views link not showing.
leaving in NW for change record.
Comment #208
smustgrave CreditAttribution: smustgrave at Mobomo commentedClosed https://www.drupal.org/project/drupal/issues/2407761 as a duplicate of this. If someone could please move over credit.
Comment #209
smustgrave CreditAttribution: smustgrave at Mobomo commentedTook a shot at the change record. Sure it will need some work but getting it moving!
Comment #211
Manibharathi E R CreditAttribution: Manibharathi E R for Srijan | A Material+ Company commentedPatch #207 tested and applied successfully on Drupal 9.5.x.
Comment #213
nod_Patch or MR doesn't apply
The last patch or MR doesn't apply to the target branch, please reroll the code so that it can be reviewed by the automated testbot.
Comment #214
smustgrave CreditAttribution: smustgrave at Mobomo commentedRerolled for 10.1.x
Comment #215
larowlanOh, hello old issue, long time no see
This would be the first usage of this namespace - is there somewhere else it could fit that already exists? e.g Drupal\Core\Entity
Or if its block specific, should it just be in the block module?
This seems to indicate it is block specific, so perhaps block module is the best spot for it?
Lower we're using $operation['url'] so should we document what the expected keys are on the returned items?
Can we add the
:array
return type hint here, thanksWe're overriding any existing query params here, should we be adding to the existing ones instead?
😀 I hope this wasn't my doing, otherwise I'm laughing at my own jokes. Edit nope it was jibran ❤️
Thanks for moving this along again @smustgrave 💪
Comment #216
gaurav-mathur CreditAttribution: gaurav-mathur at Dotsquares Ltd. commentedComment #217
gaurav-mathur CreditAttribution: gaurav-mathur at Dotsquares Ltd. commentedPatch #214 applied successfully work fine.
Comment #218
smustgrave CreditAttribution: smustgrave at Mobomo commentedTried to address #215
1. Agreed and moved the interface to the block module. Renamed BlockOperationsProviderInterface
2. Same as above
3. Tried to update the comment if that makes more sense
4. Add :array as the return type actually breaks. But using breakpoints I see the return is an array of array links.
5. This seems to be specific to block content, not sure if it's an issue?
6. I like the description haha
7. is that not covered in the block-content kernel test BlockContentTest
Comment #219
andypostThe block module is not required comparing to system and user, so this interface should live in core or in system module
not sure it's good idea to pollute core namespace with module's one
it brings dependency on block module to system one - IMO that's no-go
Comment #220
andypostCurious if this array should provide "access" for each link, like
hook_entity_operation()
doingComment #221
smustgrave CreditAttribution: smustgrave at Mobomo commentedGood point. Probably all of the scenarios should check permission right? If they have menu or views permission.
Comment #222
tim.plunkettIMO this should move to an implementation of
hook_entity_operation()
in block.moduleI think this can live in system.module. Not the best place, but better
Comment #223
smustgrave CreditAttribution: smustgrave at Mobomo commentedAddressed #220 by adding access check to all 3 spots. Also added additional test coverage if that user doesn't have correct permission.
Moved BlockOperationsProviderInterface to system.
Moved the changes from
Comment #224
smustgrave CreditAttribution: smustgrave at Mobomo commentedNot sure how the patches #223 got uploaded without being attached to a comment.
Comment #225
smustgrave CreditAttribution: smustgrave at Mobomo commented#224 had some files from another ticket.
This should be the correct one.
Comment #226
andypostlooks like it has collision with \Drupal\Core\Entity\EntityListBuilder::ensureDestination()
I think it should mention about checking access.
Probably the best approach is to add required method argument a current user
Comment #227
smustgrave CreditAttribution: smustgrave at Mobomo commentedSomething like this?
BlockContent and ViewBlock actually were using an AccountInterface already. Add to add a parameter for SystemBlockMenu.
Comment #228
joachim CreditAttribution: joachim as a volunteer commentedLooks like a very useful addition!
This hook is for adding operations to entities you don't control.
But in this case, we do control block entities. So this should be done in BlockListBuilder::getOperations() instead of in a hook.
I don't understand what's going on with this interface.
It's getting used by BOTH the entity class AND some plugins?
Clearer to say $custom_block rather than $entity.
What if the user has the 'edit all blocks' permission? This should use a call to $entity->access(), not check permissions directly.
(Aside: I'm astounded that we don't already have this functionality for custom blocks! How do users currently go to edit them?)
Same as above, I would use access() on the menu entity. There is only the one admin permission for editing menus in core, but a contrib module might add permissions per-menu.
Huh! Menu entities don't have any entity links defined! Is there an issue for this yet? There should probably be one! We should use entity links rather than hardcoded route.
Use setUpCurrentUser() for this?
This is already in the constructor. Can we avoid this repetition by setting the view and display ID as properties there?
Same - use $entity->access()?
Same - wot no entity link templates??? [1]
[1] Cultural context: https://en.wikipedia.org/wiki/Kilroy_was_here
Comment #229
Gunjan Rao Naik CreditAttribution: Gunjan Rao Naik commentedAdded patch against #227 in 10.1.x version
Comment #230
smustgrave CreditAttribution: smustgrave at Mobomo commented@Gunjan Rao Naik thank you for the interest but the patch #227 applied to 10.1 fine so #229 was not needed. Removing credit as it's a duplicate. Actually appears to be removing some of the fix too.
Comment #231
smustgrave CreditAttribution: smustgrave at Mobomo commented#228
Thank you for the detailed review.
1. I don't see a function getOperations in listBuilder?
2. Think that is correct? If you create a custom block while placing a block maybe
3. Changed variable name
4. There is no edit all block permission. But fixed
5. You would have to go under custom block library (which will soon be moved to under content so even less clear when on the block layout page)
6. Fixed
7. I don't know that but checking other examples in core and that's how they're getting the route to the specific menu.
8. We can
9. Fixed
10. Fixed
11. I don't know that but checking other examples in core and that's how they're getting the route
Comment #233
smustgrave CreditAttribution: smustgrave at Mobomo commentedFixed incorrect access check for views.
Also reverted back #231.8
Comment #234
joachim CreditAttribution: joachim as a volunteer commented> 1. I don't see a function getOperations in listBuilder?
Sorry, I got the name wrong. It's this: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...
Comment #235
smustgrave CreditAttribution: smustgrave at Mobomo commentedFixed #231.1
Comment #237
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #239
smustgrave CreditAttribution: smustgrave at Mobomo commentedmoved the getOperations check to BlockListBuilder
Comment #240
joachim CreditAttribution: joachim as a volunteer commentedSorry, I've led you down the wrong path again with this! I've had another look at the EntityListBuilder class. There *is* a getOperations method, but neither that nor buildOperations() are the right ones to override.
There are several methods on EntityListBuilder that work together:
- buildOperations() makes the render array. To get the list of operations, it calls getOperations()
- getOperations() collects the operations from the list builder, and hooks, and runs the alter hook on them all
- getDefaultOperations() defines the basic operations for the entity type in the list builder.
So it's getDefaultOperations() we should override.
It needs an @inheritdoc tag rather than repeating docs as in the current patch, and it should start by calling the parent.
Comment #241
smustgrave CreditAttribution: smustgrave at Mobomo commentedMoved to the getDefaultOperations() function.
Comment #242
joachim CreditAttribution: joachim as a volunteer commentedWe don't need this check now -- we know we're working with a block.
Would it be simpler to assign the operations from the plugin to a separate variable, loop on THAT instead, and THEN do the $operations += to merge? That way we don't need to check hardcoded array keys.
Though I'm actually not sure what this method is for -- it just passes on to the plugin.
Would it be ok for the list builder to do $block->getPlugin()->getOperationLinks() directly?
I suppose it then means the caller has to faff with checking the plugin interface. If the only caller of this code is likely to be the list builder, then that's ok. But if other code might want to get the links for a block, then yes it makes sense to have this method.
I don't think this interface serves any purpose. We're adding it to the block entity class, so it's for ALL blocks. Why not just add the new method to BlockInterface?
BC policy allows that.
Comment #243
smustgrave CreditAttribution: smustgrave at Mobomo commentedhad to move a few things around.
Comment #244
smustgrave CreditAttribution: smustgrave at Mobomo commentedMoving things around broke the Broke block
Comment #245
joachim CreditAttribution: joachim as a volunteer commentedThis seems to have taken a wrong turning. I think you're mixing up the *entity* class and interface and the *plugin* class and interface maybe?
In #242 I meant that we don't need to add a new interface for the entities. That's because we want to be able to ask ANY block to give us some links. In an admin list of blocks, we're going to ask ALL of them.
Meanwhile, it does make sense to have an interface for the *plugins*. Because some plugins provide links, and some don't.
This looks all wrong.
BlockBase is the base plugin class! The plugin here is $this! And getPluginId() gets you a string, so it can NEVER be an instance of anything!
What I meant was put it on the *entity* interface, not the plugin interface!
Comment #246
smustgrave CreditAttribution: smustgrave at Mobomo commentedBut moving getOperationLinks to BlockInterface seems to be breaking everything as some blocks don't implement getOperationLinks()
Comment #247
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis is only way I could get it to work moving to BlockInterface
Comment #248
joachim CreditAttribution: joachim as a volunteer commented> But moving getOperationLinks to BlockInterface seems to be breaking everything as some blocks don't implement getOperationLinks()
There is only one class which implements BlockInterface -- the Block entity class. See https://api.drupal.org/api/drupal/core%21modules%21block%21src%21BlockIn...
To recap:
1. We add getOperationLinks() to the interface for the *entity* class. Because there is only one entity class, and it needs that method.
2. We add a new interface which *some* plugin classes will implement.
Here we should test for the new interface. Also, this code currently won't work for contrib or custom modules ;)
I *think* that's the only main thing to fix, and also a few test code nitpicks:
If you're importing UserCreationTrait, can't you use the methods from that here?
Shouldn't it be saved?
Comment #249
smustgrave CreditAttribution: smustgrave at Mobomo commentedAdded the interface BlockOperationsProviderInterface back and added it to the plugins for BlockContent, SystemMenu, and Views
1. Updated the test to use the UserCreationTrait.
2. I don't believe so as the operation links are still correctly returned.
Comment #250
joachim CreditAttribution: joachim as a volunteer commentedThis is definitely getting there! I'd say the structure is right, now. Just some small things to clean up now:
The comment should say why we need to do this, because I have no idea!
The formatting isn't right here, and the docs should explain what each link is. Sorry -- I was too busy focussing on where methods were and didn't read the docs!
Something like:
> Each operation link is itself an array with the following keys:
- title: The title the link should display.
- url: The \Foo\Thing\Url object for the link.
- weight: The link weight.
Same here.
Performance nitpick: check moduleExists() before loading the menu entity.
Use setupCurrentUser() here?
There's no user set up at this point, is there? I don't know what kernel tests do in this situation -- is it the anon user?
Comment #251
smustgrave CreditAttribution: smustgrave at Mobomo commented1. Updated comment
2. Updated description
3. Same
4. Changed the code around to check for the module with nested if statement
5. Changed to currentUser
6. Not sure on that one but set a currentUser at the beginning of the test function
Comment #252
joachim CreditAttribution: joachim as a volunteer commentedWe've lost the first sentence. We need to first say it's an array of things, and then what each thing is.
Foo\Thing was a placeholder!!
Same as other method.
I meant that even more easily, you can use setUpCurrentUser() here can't you?
Comment #253
smustgrave CreditAttribution: smustgrave at Mobomo commentedthat was my mistake wasn't paying attention.
Comment #254
joachim CreditAttribution: joachim as a volunteer commentedLooking great!
Unfortunately I've spotted a few other small things:
Because of this, the interface's getOperationLinks() should say something about how the keys work in its @return.
Something like:
What's the reason for putting this in system module?
Block-related things tend to go in the Block namespace, or in the block module.
Why not put it alongside other block plugin interfaces in Drupal\Core\Block?
It should get an @ingroup block_api to match other interfaces there.
And also a @see to \Drupal\block\Entity\Block::getOperationLinks() would be nice.
Comment #255
smustgrave CreditAttribution: smustgrave at Mobomo commentedAttempted to address points in #254
Comment #256
joachim CreditAttribution: joachim as a volunteer commented@smustgrave pointed out to me in slack that:
> it was requested in previous comments that [the interface] be placed in system
I've gone back and read some of the discussion about this... I'm glad I didn't when I started reviewing patches here a few days ago, as I probably wouldn't have dared :/
But this issue is nearly TEN years old, it would be nice to get it in as it's a nice UX improvement.
So, regarding the interface and how generic we make it:
- I don't think this is the issue to integrate contextual links and entity operation links. For one thing, they work completely differently - contextual links are plugins, but entity operation links are simple arrays. The fact that they are different systems is a DrupalWTF, though, definitely. Needs an issue if there isn't one already.
- Making it non-specific to block plugins might be a good idea. The 'config entity + plugin' pattern is fairly common. Although, block plugins are a lot more varied than other kinds of plugins -- we get blocks derived from views, from menus, and so on. So it could be that other contrib entity + plugin type systems won't need to add dynamic operation links coming from the plugin. For instance, we don't need this in Flag module. Also, I'd rather not go down more bikeshedding when this patch feels pretty close.
So I reckon we keep it in the Block plugin namespace, as in the latest patch. It wouldn't be a big deal with later on introduce a generic interface and deprecate the Block-specific one.
Really close:
Shouldn't have 'to'.
We should also warn about the keys here.
Should be 'following keys:'.
Comment #257
srilakshmier CreditAttribution: srilakshmier at Valuebound for Valuebound commentedComment #258
srilakshmier CreditAttribution: srilakshmier at Valuebound for Valuebound commentedComment #259
srilakshmier CreditAttribution: srilakshmier at Valuebound for Valuebound commentedModified #255 based on 1 and 3 in #256. Kindly review.
Comment #261
smustgrave CreditAttribution: smustgrave at Mobomo commentedAddressed the last issue
Comment #262
joachim CreditAttribution: joachim as a volunteer commentedNearly there! Missing : here.
Comment #263
Nitin shrivastava CreditAttribution: Nitin shrivastava at OpenSense Labs commentedComment #264
Nitin shrivastava CreditAttribution: Nitin shrivastava at OpenSense Labs commentedtry to address #262
Comment #265
joachim CreditAttribution: joachim at Factorial GmbH commentedThanks! I think this is RTBC!!!
Comment #266
xjmYay!! It's really great to see this VDC usability improvement finally getting close to ready after... wow, a decade.
#256 is helpful feedback WRT the architecture and should be added to the IS. We should also probably get block subsystem maintainer signoff on that decision and the overall architecture once the following small things are cleaned up:
It sounds like this can be documented as
array[]
.Since this is a new method, we can typhint to the return value.
Sounds like this can also be documented as
array[]
.Also, why are we adding an identical method to both
BlockInterface
and a new interface? It seems like eitherBlockInterface
should be implementing the new interface, or the base class should. The current status seems redundant, with no docs as to why.An inline comment or two wouldn't hurt.
This is missing the word "that" ("Check that the current user...").
"URL" should be capitalized.
Also, set later where and how? An @see to the code that does this would help too.
Finally, it might be better to move this comment directly above the
'url'
key line for clarity.Why this weight? Comment would be good.
"up to" sbould be two words.
This is confusing at first glance. I would say:
Since these are new protected properties, they can be typehinted directly in PHP 8.
I would also really like to use constructor property promotion here, but right now that's still blocked on coding standards discussions, so I guess we best leave that aside for now. #1956134: Provide helpful editing links on "admin/structure/block" for deriver blocks (menu, views, block content, etc.)
For BC, the new constructor parameters should default to NULL and be loaded from the static container with a deprecation warning. I guess that would mean we also need to use a nullable typehint for now, which would be changed in D11. We would need to update the docs accordingly as well.
"machine_name" should be two words with an underscore.
Same feedback as before.
Finally, the change record is a bit thin; it could use some more explanation of how to use the API (plus mention the deprecation we'll be adding for the constructor).
Thanks a ton for working on this!
Comment #267
joachim CreditAttribution: joachim as a volunteer commented> Also, why are we adding an identical method to both BlockInterface and a new interface? It seems like either BlockInterface should be implementing the new interface, or the base class should. The current status seems redundant, with no docs as to why.
The combination of these methods and interfaces has gone through a lot of very different iterations and I took a change of direction when I started reviewing this issue. My thinking is this:
- code that works with block entities should be able to say 'gimme operation links for this block entity', without needing to know about gory details. Hence the method is on the block entity class, so you can do that.
- the specifics of whether a block entity has any operation links are up to the block plugin. So each plugin class can implement the interface and the method to provide links.
So:
- Block entity class getOperationLinks() -- can be called on any block entity, you'll at least get an empty array
- Block plugin classes getOperationLinks() -- only on plugin classes which want to provide links
In other words, entity::getOperationLinks() is a broker which take care of whether plugin::getOperationLinks() can be called.
Not sure how to document that, any thoughts?
Comment #268
xjmI asked @tim.plunkett and @larowlan if they could review that architecture choice.
Comment #269
smustgrave CreditAttribution: smustgrave at Mobomo commented#266
1. updated to array[] and added typehint
2. typehint added
3. updated to array[]
4. added some comments
5. add the word "that"
6. capitalized URL. Moved comment and added a little more
7. Added comment for why that weight
8. updated text
9. updated text
10. updated
11. updated and added a 2nd CR https://www.drupal.org/node/3333923
12. updated
13. updated text
Hopefully we are close because will admit losing joy in this one.
Comment #270
xjmStill need the subsystem maintainer review on the architecture and #267.
Comment #271
larowlanI'm not a subsystem maintainer for block, but hope Tim doesn't mind me removing the tag
If we add getOperationsLinks to BlockBase, we can do away with the interface and the instance of check, I think that is OK for BC sake under the 1:1 rule
- ie what @joachim said above
I think we can use \Drupal\Core\Entity\EntityListBuilder::ensureDestination on the urls here instead of this code
this doesn't appear to be used anymore?
Comment #272
joachim CreditAttribution: joachim as a volunteer commented> If we add getOperationsLinks to BlockBase, we can do away with the interface and the instance of check
I'm fine with that architecture too -- I guess it's a case of whether the perception is:
a) block plugins provide operation links in general. For those plugins that don't, the base class implements the method to return an empty array
or
b) providing operation links is something special that only some block plugins do, and so there is an interface for this
Which is quicker and clearer? I guess if we're happy to add a method to the main plugin interface and base class, then a) is better DX as a developer doesn't need to think about adding an extra interface to their plugin class.
Comment #273
xjmI agree with #271 FWIW.
Comment #274
smustgrave CreditAttribution: smustgrave at Mobomo commentedBefore redoing again
Removing the interface and the interface check seems to break because now all blocks seem to need a getOperationLinks() function now. Which is why added the interface..
Comment #275
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #276
Rinku Jacob 13 CreditAttribution: Rinku Jacob 13 at Srijan | A Material+ Company for Drupal India Association commentedI have successfully Reviewed and tested patch #275 for drupal version 10.1.x . I think this patch is very useful to edit the block from the block layout instead of gone through each pages. Adding screenshots for the reference
Comment #277
joachim CreditAttribution: joachim as a volunteer commentedThis method also needs to be on the plugin interface, not just the entity interface!
Comment #278
joachim CreditAttribution: joachim as a volunteer commented@smustgrave I'm on a train and without access to Slack, so in response to your comment there:
The latest patch is adding a method to block plugins, and calling it in on all block plugins:
+ public function getOperationLinks(): array {
+ $plugin = $this->getPlugin();
+ if (method_exists($plugin, 'getOperationLinks')) {
+ return $plugin->getOperationLinks();
+ }
+ return [];
+ }
Therefore, BlockPluginInterface must add this method.
Previous patches defined a new interface for block plugins to implement optionally:
> If we add getOperationsLinks to BlockBase
which is fine, but BlockPluginInterface must still add this method because that is part of the API we are expecting block plugins to have.
There seems to be some confusion on this issue about the entity and the plugin. Both are called 'block' which doesn't help. There are two totally separate class/interface hierarchies in play here:
- the block entity, which inherits from ConfigEntityBase, implements BlockInterface. This holds the configuration the admin user enters.
- the block plugin, which inherits from BlockBase, implements BlockPluginInterface. This holds the machinery for outputting the block.
Comment #279
joachim CreditAttribution: joachim as a volunteer commentedOh wait, it's using method_exists($plugin, 'getOperationLinks').
Let's not do that though -- it's ugly. We should add the method to the block interface.
Comment #280
smustgrave CreditAttribution: smustgrave at Mobomo commentedThen won't I have to update all block plugins now. They all seem to complain about the missing function if I add to BlockPluginInterface
Comment #281
larowlanNo, just add an empty implementation to BlockBase, all blocks are expected to extend from it
Comment #282
smustgrave CreditAttribution: smustgrave at Mobomo commentedLike this?
Comment #283
smustgrave CreditAttribution: smustgrave at Mobomo commentedHad to add a stub to Broke.php.
Comment #284
smustgrave CreditAttribution: smustgrave at Mobomo commentedAll green!
Comment #285
joachim CreditAttribution: joachim as a volunteer commentedThis needs an inherit doc. I'm surprised the code checking didn't complain about it!
We don't need to check the method exists, since it's on the block plugin interface. Every block plugin *must* implement this.
Comment #286
smustgrave CreditAttribution: smustgrave at Mobomo commentedFixed points in #285
Think the issue summary is fine but am working on CR updates
Comment #287
joachim CreditAttribution: joachim as a volunteer commentedYay! I think this is ready!
I don't think we need this CR:
> SystemMenuBlock uses ModuleHandlerInterface and AccountInterface
The BC policy says:
> Particular plugins, whether class based or yaml based, should not be considered part of the public API. References to plugin IDs and settings in default configuration can be relied upon however.
Comment #288
quietone CreditAttribution: quietone at PreviousNext commentedUnfortunately, the patch is not passing the checks.
Comment #289
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #290
larowlanFirst up, I apologize in advance for asking some awkward questions here. But I'm concerned about the implicit dependencies this adds between the system module and the menu UI module, as well as the views module and the views UI module.
But first some minor comments
Per review above from @andypost and myself, I think this should use the existing ::ensureDestination method on this class
This comment doesn't match the code, 50 would put it at the bottom.
I don't think we should test this with user 1. So perhaps create it with a UID > 1
User 1 typically bypasses access checks
It will require granting the 'administer block' permission too, but then we're getting closer to a real test
Ok, brace yourself, this is where I get annoying.
I'm not convinced system module should know about the menu UI module.
And perhaps that suggests we're missing an API here.
For example, let's say module A wants to modify the operation links of module B. e.g. in this case menu_ui would want to edit the operation links of system module.
But then when you think about it, we already have a hook for adding and altering operation links - that's
hook_entity_operation
andhook_entity_operation_alter
So if menu_ui cares about an operation for a block provided by system module, it should handle it itself, not the other way around.
So I think this change should be reversed, and menu_ui module should implement one of those hooks, probably the first one and add this edit link from there.
And the same comment here re views ui / views module and the existing hook
So yeah, those last two points are annoying, and sorry they're coming at comment #290. But one good thing is, the test already exists, so the impact shouldn't be as great.
The added bonus of that is we don't need any API changes for those two block plugins, which are commonly extended (yes they're internal, but in reality they do get extended a lot).
Thanks again for keeping this moving @smustgrave, and once again sorry for pushing back this late
Comment #292
Mutasim Al-Shoura CreditAttribution: Mutasim Al-Shoura at Vardot commentedI have updated patch #289 and relocated the edit block link to the top of the list on the block layout page.
Comment #293
xjmThanks for your help updating this patch. In the future, when using a patch workflow, provide interdiffs for your patches. That allows reviewers to evaluate your changes.
Comment #295
smustgrave CreditAttribution: smustgrave at Mobomo commentedPushed up some of the feedback from #290.
290.3 will probably need all the tests redone especially if we do 290.4
As far as using the hooks, have to do some trickery to get the name of the menu. Not sure can figure out the view name/display.
So we sure this is the approach to take?
Comment #296
larowlanWe should have the block entity in scope.
From there we can get the base block plugin ID.
If the plugin ID is system_menu_block
Comment #297
smustgrave CreditAttribution: smustgrave at Mobomo commentedAddressed feedback from #290
Comment #298
Wim LeersAnother round of feedback 😊 Looking great in general, just relatively nitpicky things (one pattern of which is an actual bug though), including suggestions on how to harden the test coverage.
Comment #299
smustgrave CreditAttribution: smustgrave at Mobomo commentedAddressed feedback.
Comment #300
larowlanLeft some comments.
This is looking really tidy now, much smaller diff, nice and clean
Comment #301
smustgrave CreditAttribution: smustgrave at Mobomo commentedAddressed feedback.
Comment #302
larowlanLeft another review
Comment #303
smustgrave CreditAttribution: smustgrave at Mobomo commentedAddressed feedback.