Change Details

MediaWiki's handling of global blocks is weird in many ways: * `User::getBlock()` does not return global blocks, you need to call `User::getGlobalBlock()` and there are lots of places in our codebase where this doesn't happen. Same for `User::isBlocked()` etc. The signatures are different, you can't force a primary DB read for global blocks etc. * `User::getGlobalBlock()` says "//Do not use for actual edit permission checks! This is intended for quick UI checks.//" so even calling both isn't safe (although looking at the code I don't see why it wouldn't be, unless it refers to the lack of READ_LATEST support, which I don't think is realistically important). What should who do instead is unclear. (Use PermissionManager, I guess, but sometimes we specifically want to know whether the user is blocked or not.) * Passing a global block to BlockErrorFormatter results in a completely wrong block message. (Non-global blocks coming from an extension do, too.) You either need to figure out which extension owns the block and what methods that extension exposes for block formatting, or use `$block->getPermissionsError()` which is deprecated and has a different signature with more limited options (you can get around that using DerivativeContext, but ew). * I don't think there's a way to get a list of blocks (whether for a specific target or in general). Global blocks are integrated to some special pages via the `OtherBlockLogLink`/`OtherAutoblockLogLink` hook, but that returns HTML fragments, not blocks. Ew. * `User::spreadBlock()` and similar methods only apply to core blocks, even though in theory it makes sense for a blocking-related extension to want to handle them. * It's unclear what methods a global block is supposed to support. E.g. GlobalBlock is a subclass of DatabaseBlock but calling most DatabaseBlocks would result in crazyness (e.g. `GlobalBlock::delete` would delete the local block which happens to gave the same ID - granted, that method shouldn't exist in the first place and Block objects should be value objects). Most permission-related methods like `GlobalBlock::isCreateAccountBlocked` give wrong results. * There is no good way to tell if a Block object is a global block if you don't know where it came from. (Should core even have a concept of global blocks, as opposed to just blocks provided by some extension? Not sure. So maybe this isn't a problem.) * The `Block::TYPE_*` nomenclature isn't extensible (or at least isn't documented to be, and it seems hard to coordinate that for integers), even though there is no reason a block would always be based on user or IP. (A bit of an academic problem though, as we don't actually have other block types.) The ideal solution would be to just merge global blocks into normal blocks: * `User::getBlock()` et al would also check for global blocks. The GetUserBlock and UserIsBlockedGlobally hooks should be merged. * BlockErrorFormatter should handle non-core blocks via its own set of hooks. * GlobalBlock should probably subclass AbstractBlock directly, and share code as needed with DatabaseBlock via a trait/interface. (At a glance the only methods it needs are get/setBlocker, although if {T17294} gets done it would probably want to implement autoblocks too.) * There should probably be a hook to get all blocks for a given set of filters, and special pages should list global blocks based on that, although this might be (way) more effort than it's worth.

MediaWiki's handling of global blocks is weird in many ways: * `User::getBlock()` does not return global blocks, you need to call `User::getGlobalBlock()` and there are lots of places in our codebase where this doesn't happen. Same for `User::isBlocked()` etc. The signatures are different, you can't force a primary DB read for global blocks etc. * `User::getGlobalBlock()` says "//Do not use for actual edit permission checks! This is intended for quick UI checks.//" so even calling both isn't safe (although looking at the code I don't see why it wouldn't be, unless it refers to the lack of READ_LATEST support, which I don't think is realistically important). What should be done instead is unclear. (Use PermissionManager, I guess, but sometimes we specifically want to know whether the user is blocked or not.) * Passing a global block to BlockErrorFormatter results in a completely wrong block message. (This is also true for non-global blocks coming from an extension, such as #regexblock or #mediawiki-extensions-torblock.) You either need to figure out which extension owns the block and what methods that extension exposes for block formatting, or use `$block->getPermissionsError()` which is deprecated and has a different signature with more limited options (you can get around that using DerivativeContext, but ew). * I don't think there's a way to get a list of blocks (whether for a specific target or in general). Global blocks are cross-linked from some special pages via the `OtherBlockLogLink`/`OtherAutoblockLogLink` hooks. Given that extensions can chose different arbitrary mechanisms for storing blocks, it might not be realistic to do more than that. It would be nice though. Also, it would be nice to cross-link other pages (like block/unblock pages or block list pages) as well. (FWIW there is some ongoing work related to navigation between special pages in {T313349}.) * `User::spreadBlock()` and similar methods only apply to core blocks, even though in theory it makes sense for a blocking-related extension to want to handle them. * It's unclear what methods a global block is supposed to support. E.g. GlobalBlock is a subclass of DatabaseBlock but calling most DatabaseBlocks would result in crazyness (e.g. `GlobalBlock::delete` would delete the local block which happens to gave the same ID - granted, that method shouldn't exist in the first place and Block objects should be value objects). Most permission-related methods like `GlobalBlock::isCreateAccountBlocked` give wrong results. * There is no good way to tell if a Block object is a global block if you don't know where it came from. (Should core even have a concept of global blocks, as opposed to just blocks provided by some extension? Not sure. So maybe this isn't a problem.) * The `Block::TYPE_*` nomenclature isn't extensible (or at least isn't documented to be, and it seems hard to coordinate that for integers), even though there is no reason a block would always be based on user or IP. (A bit of an academic problem though, as we don't actually have other block types.) The ideal solution would be to just merge global blocks into normal blocks: * `User::getBlock()` et al would also check for global blocks. The GetUserBlock and UserIsBlockedGlobally hooks should be merged. (I think there is one situation where a separate global block hook could be useful: when checking whether a user with no local account on the wiki is blocked. This is a very particular use case that's only needed during account creation / autocreation, though; and the current global block hook doesn't really support it.) * BlockErrorFormatter should handle non-core blocks via its own set of hooks. * GlobalBlock should probably subclass AbstractBlock directly, and share code as needed with DatabaseBlock via a trait/interface. (At a glance the only methods it needs are get/setBlocker, although if {T17294} gets done it would probably want to implement autoblocks too.) Also the Block/AbstractBlock separation does not seem useful as it is and should probably be reworked so there's an interface that can actually be used for implementing new block types. * Maybe there should be a hook to get all blocks for a given set of filters, and special pages should list global blocks based on that, although this might be (way) more effort than it's worth.

MediaWiki's handling of global blocks is weird in many ways: * `User::getBlock()` does not return global blocks, you need to call `User::getGlobalBlock()` and there are lots of places in our codebase where this doesn't happen. Same for `User::isBlocked()` etc. The signatures are different, you can't force a primary DB read for global blocks etc. * `User::getGlobalBlock()` says "//Do not use for actual edit permission checks! This is intended for quick UI checks.//" so even calling both isn't safe (although looking at the code I don't see why it wouldn't be, unless it refers to the lack of READ_LATEST support, which I don't think is realistically important). What should who dobe done instead is unclear. (Use PermissionManager, I guess, but sometimes we specifically want to know whether the user is blocked or not.) * Passing a global block to BlockErrorFormatter results in a completely wrong block message. (NThis is also true for non-global blocks coming from an extension do, toosuch as #regexblock or #mediawiki-extensions-torblock.) You either need to figure out which extension owns the block and what methods that extension exposes for block formatting, or use `$block->getPermissionsError()` which is deprecated and has a different signature with more limited options (you can get around that using DerivativeContext, but ew). * I don't think there's a way to get a list of blocks (whether for a specific target or in general). Global blocks are integrated tocross-linked from some special pages via the `OtherBlockLogLink`/`OtherAutoblockLogLink` hook,s. Given that extensions can chose different arbitrary mechanisms for storing blocks, it might not be realistic to do more than that. It would be nice though. but that returns HTML fragmentsAlso, notit would be nice to cross-link other pages (like block/unblock pages or blocks list pages) as well. Ew.(FWIW there is some ongoing work related to navigation between special pages in {T313349}.) * `User::spreadBlock()` and similar methods only apply to core blocks, even though in theory it makes sense for a blocking-related extension to want to handle them. * It's unclear what methods a global block is supposed to support. E.g. GlobalBlock is a subclass of DatabaseBlock but calling most DatabaseBlocks would result in crazyness (e.g. `GlobalBlock::delete` would delete the local block which happens to gave the same ID - granted, that method shouldn't exist in the first place and Block objects should be value objects). Most permission-related methods like `GlobalBlock::isCreateAccountBlocked` give wrong results. * There is no good way to tell if a Block object is a global block if you don't know where it came from. (Should core even have a concept of global blocks, as opposed to just blocks provided by some extension? Not sure. So maybe this isn't a problem.) * The `Block::TYPE_*` nomenclature isn't extensible (or at least isn't documented to be, and it seems hard to coordinate that for integers), even though there is no reason a block would always be based on user or IP. (A bit of an academic problem though, as we don't actually have other block types.) The ideal solution would be to just merge global blocks into normal blocks: * `User::getBlock()` et al would also check for global blocks. The GetUserBlock and UserIsBlockedGlobally hooks should be merged. (I think there is one situation where a separate global block hook could be useful: when checking whether a user with no local account on the wiki is blocked. This is a very particular use case that's only needed during account creation / autocreation, though; and the current global block hook doesn't really support it.) * BlockErrorFormatter should handle non-core blocks via its own set of hooks. * GlobalBlock should probably subclass AbstractBlock directly, and share code as needed with DatabaseBlock via a trait/interface. (At a glance the only methods it needs are get/setBlocker, although if {T17294} gets done it would probably want to implement autoblocks too.) Also the Block/AbstractBlock separation does not seem useful as it is and should probably be reworked so there's an interface that can actually be used for implementing new block types. * There should probably* Maybe there should be a hook to get all blocks for a given set of filters, and special pages should list global blocks based on that, although this might be (way) more effort than it's worth.