Needs a rebase now 📌 Convert ckEditor5CodeSyntaxTest to WebDriver test Active is in
One outstanding comment which I'm not sure on whether it can be resolved.
Working on the reroll
I've merged 11.x into this branch and resolved all conflicts. I've reverted some of the code that removed datetime_wrapper theme hooks and preprocessing and I think we need to add the templates back as well and deprecate that properly.
acbramley → created an issue.
Just noticed when rebasing 🐛 Datetime and Datelist elements should render as fieldsets Needs work that all of the functions called from date related preprocessors are calling BC code incorrectly.
function template_preprocess_datetime_form(&$variables): void {
@trigger_error(__FUNCTION__ . '() is deprecated in drupal:11.2.0 and is removed from drupal:12.0.0. Initial template_preprocess functions are registered directly in hook_theme(). See https://www.drupal.org/node/3504125', E_USER_DEPRECATED);
\Drupal::service(ThemePreprocess::class)->preprocessDatetimeForm($variables);
}
preprocessDatetimeForm does not exist on that class, rather it's on \Drupal\Core\Datetime\DatePreprocess
Will open an issue.
acbramley → changed the visibility of the branch 3078334-datetime-and-datelist to hidden.
acbramley → changed the visibility of the branch 3078334-datetime-and-datelist-945 to hidden.
acbramley → changed the visibility of the branch 3078334-datetime-and-datelist-95x to hidden.
@christian.wiedemann Thank you for committing this, can we please get a new release on the 1.0 branch that includes this fix?
Updating the current and proposed text to show more of what we could replace in the docs.
Sniffs that would need changing:
For allowing the omission of @var comments - Drupal.Commenting.VariableComment.Missing. You are currently allowed to omit the @var if the variable is typed, but you still need a comment. It's likely this logic can be copied to the sniffs below.
For allowing the omission of @return when a return type exists - Unsure, this currently does not throw an error with either or both missing, only when the @return tag exists without a type. Likely need changes in Drupal.Commenting.FunctionComment.MissingReturnType
For allowing the omission of @param when a param type exists - this currently does not throw an error with either or both missing, only when the @param tag exists without a type. Likely need changes in Drupal.Commenting.FunctionComment.MissingParamType
Perfect, in that case I've reverted all the presave code.
99% of this code is in content_moderation. Moving to that queue.
This fails 100% of the time for me locally.
Debugging into it I see the row in the migrate_map table, but the config table only has a single entry for that block (i.e the default language)
MySQL [local]> select * from test72763259migrate_map_d7_block_translation;
+------------------------------------------------------------------+-----------+-----------+-------------------+---------+-------------------+-----------------+---------------+------+
| source_ids_hash | sourceid1 | sourceid2 | destid1 | destid2 | source_row_status | rollback_action | last_imported | hash |
+------------------------------------------------------------------+-----------+-----------+-------------------+---------+-------------------+-----------------+---------------+------+
| 1eb693d2354c0b014b1ed9fe9acc754b00362495e1b8b25aa25c53304192f477 | login | fr | bartik_user_login | fr | 0 | 0 | 1745988771 | |
+------------------------------------------------------------------+-----------+-----------+-------------------+---------+-------------------+-----------------+---------------+------+
1 row in set (0.000 sec)
There are also some error messages for the parent migration
MySQL [local]> select * from test62552709migrate_message_d7_block;
+-------+------------------------------------------------------------------+-------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| msgid | source_ids_hash | level | message |
+-------+------------------------------------------------------------------+-------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| 1 | 70bb1a3d2a830bbad0d4625652cbab5bbb9e8cb8c20e32aee04d68791a8a7678 | 1 | Schema errors for block.block.seven_system_main with the following errors: 0 [dependencies.theme.0] This value should not be blank., 1 [dependencies.theme.0] Theme '' is not installed., 2 [theme] This value should not be blank., 3 [theme] Theme '' is not installed., 4 [region] This value should not be blank., 5 [region] This block does not say which theme it appears in. |
| 2 | c0e5f09290df5ef8dc83c8f534dbcbd1085182faf6af9706e5e84e541f4d5c84 | 1 | Schema errors for block.block.seven_user_login with the following errors: 0 [dependencies.theme.0] This value should not be blank., 1 [dependencies.theme.0] Theme '' is not installed., 2 [theme] This value should not be blank., 3 [theme] Theme '' is not installed., 4 [region] This value should not be blank., 5 [region] This block does not say which theme it appears in. |
+-------+------------------------------------------------------------------+-------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
2 rows in set (0.000 sec)
This is actually in block module, the test class is just misnamed.
Random failure being tracked here 🐛 [random test failure] Drupal\FunctionalTests\Bootstrap\UncaughtExceptionTest::testLostDatabaseConnection Active
After all that I'm actually unsure how we should do this. With this presave hook it's impossible now to save a role with administer nodes and without rebuild node access permissions...
Throwing a deprecation didn't feel right either because it's a valid case to have one permission without the other, but we need to be able to notify modules with exported config.
@catch any ideas?
Added the presave hook but debugging locally and it doesn't fire during the post update...
This is so inconsistent.
The latest commit passed 100/100 times on a Repeat run but failed in the normal test run.
Then I reran the Repeat run and it failed every single time.
Actually, the message in the failure is
PDOException: SQLSTATE[HY000] [1524] Plugin 'mysql_native_password' is not loaded in Drupal\Component\DependencyInjection\PhpArrayContainer->createService() (line 77 of core/lib/Drupal/Component/DependencyInjection/PhpArrayContainer.php).
I forgot the test case after the failed one still spits out HTML output.
HTML output in the failed test is
Exception: Deforestation in Drupal\error_service_test\MonkeysInTheControlRoom->handle() (line 69 of core/modules/system/tests/modules/error_service_test/src/MonkeysInTheControlRoom.php).
So I think this is a state cache bug from one of the other test cases. Will swap to use KV
acbramley → created an issue.
Just need to rebase it or push a new commit.
You can comment /rebase on the MR to do that.
Updated title. I think a new permission is the best approach here, if we used administer site configuration we'd still need an upgrade path.
Keeping at NW because I think we do need the ConfigUpdater dance here in a presave hook
No further comms so this can be closed.
@ckspringbok if you have steps to reproduce this issue please feel free to reopen.
In step 3 are you creating that block as a reusable block in the block library, or in the Node itself as an inline block?
From this quote it sounds like the former:
The default block content from within the content type layout should remain unchanged when making block content changes on the new node
If this is the case, this is works as designed. Editing a reusable block via a Layout form using inline entity form is still editing that same reusable block.
Yes please, this has failed multiple times in a row on another branch.
The test here caused a very frequent random fail 🐛 [random test failure] jQuery Events Deprecation Tests (Tests/dialogDeprecations) Active let's try to a) be careful with random ms waits in tests (wait for elements instead) and b) not use nightwatch wherever possible.
if node module implements a config schema change on node_type, the shipped node types config of contrib module X are not updated on installation of module X
We're starting to tackle that now with pre_save hooks (see issue below)
I've opened 📌 Add generic interface + base class for upgrade paths that require config changes Active which is sort of similar to this issue but for config entity changes specifically.
WRT. this issue, Layout builder is going to be really tricky here for overridden entities. That config is stored as a blob in the database. It's a massive headache to update these and requires an entity save or some DB voodoo.
I wonder if instead we have something that applies default config upon loading the plugin? That way when a user edits an entity that has a block plugin in its overridden LB storage, it'll get those config changes applied and then when they save the changes will persist to the db?
acbramley → created an issue. See original summary → .
Rolled this into an MR and:
1. Reverted the test changes, not sure if we need any additional coverage
2. Added a deprecation to the query_alter hook
3. Swapped to attributes
4. Added ::validateReferenceableNewEntities to the new plugin, I don't think we need ::createNewEntity since reusable defaults to TRUE
acbramley → made their first commit to this issue’s fork.
Re #54
Thanks for the detailed write-up, lots of great insight in there.
As a subsystem maintainer of block_content I'm a little on the fence on whether this feature should be in core. As you say, blocks aren't really designed to be rendered by themselves. However, I have implemented this exact thing for a client who wanted to be able to see a rough idea of what the block would look like on a dedicated page.
Responding to some of the feedback:
The tab should be renamed to Preview, view you associate with viewing something in its final form
I don't think Preview fits here either, and may be confusing for some users. We have a Preview button on Nodes which takes you to an unsaved version of the Node and allows navigating back to the edit form without losing changes.
If we have a Preview tab for Block content, users may confuse this functionality and click on the tab which would effectively lose their changes in the form.
And in regards of the preview it would be a reasonable step to use the admin theme Claro instead of the default theme Olivero respecting the context of the /admin/content/block/ path - using Olivero per default is waking wrong expectations as well as assumptions.
I think this would defeat the purpose of the feature for a lot of users as the block would look nothing like what it is going to look like in the Layout Builder or Block plugin context. It should be in the frontend theme.
Closing as per #4
I think it was a mistake to combine this with #2909435: Update block library page to adapt publishable block content implementation → . I feel like splitting the views changes and the entity form changes would make this much easier to get in tbh, especially considering we don't have any upgrade paths here so existing sites will be unaffected and therefore the consistency argument is semi-redundant?
Trying to figure out what the way forward here is.
But #53 points to #34 which is going to be quite disruptive as per #35
People would then also need to add defence when parameters aren't defined otherwise static analysis tooling such as phpstan will complain on higher levels.
I like the idea in #73, funnily enough core's CsrfAccessCheck already aliases AccessInterface as RoutingAccessInterface.
The sentence in question doesn't seem to belong in block content's help anyway so IMO we can just remove that therefore this is back to NR.
Triaged as part of BSI, part of me wants to make this a task as it's just documentation clarification but I can see some people viewing this as a bug.
This contradicts 🐛 Views handler loading should respect configuration Active and that issue is doing the opposite - enforcing the plugin is used via config.
This is good to go again.
CR added, I'm not sure if we also need a presave hook here for exported config in modules/profiles
Would be good to get another set of eyes on this.
I've removed the interface now so that's taken care of
Last decision needed for https://git.drupalcode.org/project/drupal/-/merge_requests/11828#note_50...
Reroll looks good @mohit_aghera just one question about the wrapper there, the IS also has conflicting statements about this (whether we're removing it or not)
Functional test failure also seems related.
I'm not a fan of dumping a huge number of assertions into such a generic test case where it's not really clear what the intention of the test is.
However, there is testNodeMetaInformation which seems fitting for this so I've moved it there.
@catch thanks, I think it's best to be explicit.
I think this is good to go now, not sure if I'm allowed to mark this RTBC or not.
so it doesn't make sense to move the functions elsewhere and refactor them for now
It doesn't make sense to add new methods to an interface/class that we know is being deprecated. We can postpone this issue on that one instead if you like.
since all class methods currently use db queries, it should be the same here
That code is ancient and is part of the reason why that class is being deprecated, it's not compatible with all db engines. We should do it properly first since we're introducing new functionality, not leave it to a follow up to refactor again.
Reading the proposed text it doesn't seem like we're enforcing that a param or return doc can only be excluded if the method has strict types for either the param or return?
I.e we should not allow removing a return doc if it does not have a return type, same for params.
I'm also unclear how this is relevant:
The parameter name is derived directly from the type name
🐛 Deprecation PHP 8.4 Active existed months before this issue and had an MR up in January. It might be good to credit those there that contributed too.
Adapted #19 into a new MR with the addition of the deviantart example, although that does match still so the preg_quote may be helping us there?
acbramley → changed the visibility of the branch 3256828-chainflix-oembed-endpoint to hidden.
acbramley → changed the visibility of the branch 11.x to hidden.
Double checked and these are the only 2 in the module. This is good to go.
MR is rebased but core pipelines are having issues atm for subsystem maintainers. If someone can click rerun on the pipeline that'd be great.
There's many out of scope changes here going from constructor injection to setting class props in ::create. All of that needs to be reverted.
Bumping this
Looks like the pipeline issues will be fixed by 🐛 PHPunit tests are failing on Drupal 11 Active
acbramley → changed the visibility of the branch 3498296-implicitly-marking-parameter-2.x to hidden.
acbramley → changed the visibility of the branch 8.x-3.x to hidden.
We need to move these new methods somewhere else, NodeStorage is in the process of being deprecated. 📌 Create a new NodeRepository for the code from Drupal\node\NodeStorage Active
We also should be using entity queries instead of direct db queries.
Uploading sample stacktraces
I tested the exception backtrace with and without the getPrevious() and IMO (for the issue described in the IS at least with missing a title) I find the backtrace without the getPrevious easier to parse.
Adding before/after screenshots.
So maybe we could just add $e = $e->getPrevious() ?: $e, at least until we have a better default handling of the backtrace including previous exception(s).
From #72
I'm guessing this is it? I've pushed that. Having issues getting gitlab to rerun the failed tests (assuming a random) but this should be ready for a re-review.
The original scope here was fixing implicit nullable deprecations which has already been done.
I think it we should do that
Are you able to expand on exactly what we should do?
Came up as a random BSI triage. As per #8 we can rescope this as a documentation issue.
Given the lack of updates here I'm going to close this one out.
Please feel free to reopen with an updated issue summary including steps to reproduce if this still affects you.
Needs a reroll onto an MR.
The IS also needs an update to use the standard template.
Currently the following code evaluates to FALSE for statically cacheable entity types, but I think that it should evaluate to TRUE both for consistency and performance
That would only be the case if nothing at all changed on the entity, right? There is probably a very small set of entity types that this would apply to, and nothing in core afaik.
You would need to:
- Have no changed date
- No revisioning
- No other data changed between saves.
I agree that the example in the IS with entity reference fields is probably an edge case that someone could run into, but it seems unlikely to do that in actual runtime code?
Added some basic context to the NodeGrantDatabaseStorageInterface::writeDefault doc block, and linked to that from NodeAccessControlHandlerInterface::writeDefaultGrant (personally I'd love to remove this duplication but that's for another issue).
I don't think we need inline documentation as it should be covered by the doc block.
Last bit I'm not sure on is this:
Clarify the default grant behavior in the node access group documentation.
Where is the node access group documentation? Maybe that no longer exists. We have quite a lot of docs at the top of https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/node/..., maybe that's sufficient?
Re #17
If it just does entity queries, then one option IMHO is to not add an interface because there isn't really a reason to provide a completely different implementation.
Happy to remove the interface, I agree but this was just following patterns already in core where it seems like every service has an interface that could often fall into the same bucket of not really making sense.
The referenced issue could also add its own service/class, that's definitely easier to deal with for BC.
BC for what? I'm a bit confused why adding new methods to a service would be hard for BC.
Creating new MRs for rebases is not ideal, now reviewers/contributors don't know where to look. Same goes for uploading patches for various fixes, now it's not clear what the canonical source is. We need to make sure the fixes from the patches in #119 and #120 are valid and they are rolled into the canonical MR.
Closing the old MR for now.
acbramley → changed the visibility of the branch 3274635-upstream-use-ckeditor to hidden.
I'm not sure, the way I read it was that the current solution is fine for this. I haven't looked for another issue but that doesn't need to block this.
Test failure was a random.
This should be fixed in the issue that introduces the feature ✨ "address" field type updates not appearing Active
@smustgrave I don't feel like testNodeMetaInformation is the right place to test the visibility of the Published checkbox. I prefer test cases to be succinct in what they test rather than having massive test cases asserting all sorts of things. It makes it easier to maintain and understand why something failed when it does.
Re #148 I think we need a review with fresh eyes. This was put back into NW in #118 with a bug in masquerade but @berdir pointed out this may be a bug in the module itself in #134
However, we may need to rethink this entirely based on that comment as well. I agree it's not the greatest solution, especially given the amount of seemingly unrelated changes made to get this to work.
Another one bites the dust 🧨
I think the main thing here is the service/class name. Otherwise this looks good to go.
Not a fan of conflating deprecation fixes for PHP 8.4 with other coding standards cruft, but this would be good to get in.