🇦🇺Australia @acbramley

can we add simple test coverage that if you use any of these deprecated constants you get a warning

I don't think this is possible, at least I can't think of how to trigger a warning.

I had a very brief look at the MR and see that it needs to be updated to meet the Drupal Coding Standards for constants

Is this relating to this part of the standards: "Module-defined constant names should also be prefixed by an uppercase spelling of the module that defines them."? Because IMO that should not apply to constants on classes. Adding the module prefix would just duplicate what's already provided by the classname, e.g NodeTypeInterface::NODE_PREVIEW_OPTIONAL. Perhaps the docs need an update?

I've merged 11.x into the branch and updated a few more spots that were missed or added since the last merge.

acbramley → changed the visibility of the branch 11.x to hidden.

I would prefer to fix this as part of 📌 Each class using DRUPAL_* constants from system.module should define its own constants Needs work for a much cleaner solution.

NW based on the comments in the test.

acbramley → made their first commit to this issue’s fork.

Ok thanks, how do we determine if a deprecation doesn't need a CR?

CR added

Updated the field description with the following:

As expected this complicates things quite a bit and is probably going to create a lot of churn on this issue. I think linking to the form display page is great for UX but complicates things a fair bit. Also what if field_ui is disabled? There is no manage form display so do we need to provide a different message again?

Anyway, it's there now so let's see what people think :)

I did search for pageTextContains(\sprintf and didn't find any instances in core

Probably because hardly anyone uses the backslash. Look for sprintf in anything *Test.php and you'll find plenty :)

https://git.drupalcode.org/search?group_id=2&nav_source=navbar&project_i...

. So do we need an upgrade hook to retroactively fix that

I don't think that's a good idea, that could be tens or hundreds of thousands of revisions.

@mohit_aghera I've reverted this back to before your changes as they were causing the bugs in #76.

I can't seem to reproduce the duplicate ajax_page_state[libraries] query param on HEAD anymore, although I suspect it might be my browser hiding it (perhaps it's merging the dupes on display in the network tab?)

I can reproduce the issue in the IS though so maybe not, the MR fixes that at least.

Looking into the test failures and I can reproduce the issues manually as well as in the tests. The problem is around clearing filters, submitting an empty filter just reloads the view with the existing filter value.

All other performance data are expected I suppose as we're adding bytes to the scripts.

Merged in 11.x which conflicted because of https://git.drupalcode.org/project/drupal/-/commit/c8c538e760e2565772b22...

No rebase as there were massive conflicts since the branch had been merged previously.

acbramley → changed the visibility of the branch 11.x to hidden.

acbramley → changed the visibility of the branch 2508796-query-string-is to hidden.

@berdir mentioned in slack that some of these methods could actually be deprecated without replacement so this will need a bit more investigation to see if there are existing methods that could replace them.

Quoting @berdir:

for example revisionIds(), the new generic revision controller doesn't use that, it uses an entity query afaik

Can someone confirm this is definitely still an issue even after the fix in 🐛 The content overview Views view filters out unpublished content Active ?

I agree with #108 here, given we will eventually have a generic revision UI, node shouldn't really be doing its own thing with a view. We'd then need a view for every revisionable entity type, and then any non-core entity type would need to implement its own view otherwise the revisions UX would be different - this is bad for editors.

This doesn't seem like something core should support IMO, it could be something explored in contrib.

I'm inclined to set this to won't fix, but given the history here I want to see what others think.

@quietone asked for some more research into when this was added.

It was added in https://git.drupalcode.org/project/drupal/-/commit/129c8eb18c47bf7b0e0fe...

The title of that commit is "Configurable node types" - i.e when node types became configurable. Part of a larger issue #62340: Pave the way for CCK →

The original function did this

db_query("UPDATE {node} SET type = '%s' WHERE type = '%s'", $type, $old_type)

I think we all know how far we've come to understand this type of operation is simply not easily possible in modern Drupal, and therefore should be removed entirely.

The IS describes this a bit as well.

After this, it stayed mostly in this state even as far as 2013 with https://git.drupalcode.org/project/drupal/-/commit/46973e74b5f5bbe278053...

The function was then deprecated in https://git.drupalcode.org/project/drupal/-/commit/d9059c0650e49da532d42... and moved to NodeStorage

Test up, was a bit tricky to get this working tbh

Looking into this further, I'm not really sure how we would even start to unpick this. It's much more baked in than just node_install

1. NodeGrantDatabaseStorage::writeDefault does the same thing. This is called from NodeAccessControlHandler::writeDeafultGrant which is called by node_access_rebuild when there are no hook_node_grants implementations
2. In NodeGrantDatabaseStorage::access we specifically add an OR condition on nid = 0 when the node is published

Let's tie this to the new NodeRepository service that 📌 Create a new NodeRepository for the code from Drupal\node\NodeStorage Active will introduce, given that the code this issue would replace directly calls functions from that service (userRevisionIds)

I also tracked this @todo down originally to #8: Let users cancel their accounts →

I'm not really sure what the goal is here but we'll need a rewrite of the IS.

The default frontpage in Standard install profile has no h1/page title

That's not true, the h1 and page title is "Welcome!". IMO the Standard install's homepage has a lot of useful information on it now.

If anything, I guess we could expand the line "You haven’t created any frontpage content yet."

Here's a screenshot of the current Standard homepage.

You can disable the Frontpage view, is there anything else you need?

+1, this is completely redundant documentation and just makes it harder to read. If parameters need documentation it should be in the constructor's doc block.

think it makes sense to extend that a bit and include something like "if they are enabled on the form display settings" or so, either as part of that sentence or an additional sentence. maybe also mention that some are not visible by default somehow.

I somewhat agree, but given this is a 20 year old issue, maybe we do that in another follow up? Because as we know wording can be hard to get right and would need a round of UX review.

I've updated the IS and CR a bit to be more explicit about the BC break

You don't need to set it to NULL, that's not what the scenario is about. It's about either setting it to a non-existing UID

Tried that and even that's not possible, we have this in Node::preSave

      // If no owner has been set explicitly, make the anonymous user the owner.
      if (!$translation->getOwner()) {
        $translation->setOwnerId(0);
      }

But yeah I agree, this is more than enough :)

I agree with moving search stuff to the search module, but it looks like this hook is directly invoked from the node_search search plugin, so maybe it does belong in node? Who knows how often this stuff is actually used though, maybe we can eventually move node search stuff to a node_search contrib module.

In any case, this needs a reroll on to an MR which is going to require quite a bit of rework for OOP hooks I imagine.

It's actually quite difficult to contrive a scenario with core to hit a node add/edit page with a NULL author.

The easiest way for node/add is to override the default value callback to stop setting it to the current user.

I tried to get edit form coverage as well but as soon as you delete the user, the node is gone too because of NodeHooks1::userPredelete

The uid column is non-NULL so we can't update the db directly.

As for wording when the user is missing, I think we can just accept an empty string here hopefully? Seeing as this is quite a rare scenario I think as long as we don't WSOD that's good enough. The author will be set on next save anyway.

Working on this, closing previous MR as the branch can't be rebased easily and it was against 11.1.x

acbramley → changed the visibility of the branch 3161212-node-add-gives to hidden.

acbramley → made their first commit to this issue’s fork.

Revert MR up

I also confirmed this issue with other field types such as Link

The further I dive into this the more I'm seeing that this should be reverted (x-posted with @catch).

We do want to use the Numeric plugin for displaying aggregation data, configuring entity field based formatter settings that get applied to the aggregated value is never going to work properly for all cases.

The real bug here is why the Numeric plugin isn't being used when the field is first being added - this is what causes the original WSOD error because it's using EntityField::buildGroupByForm, then the next time you edit it it's using the parent (HandlerBase) and submitGroupByForm doesn't have the group_columns fields which then passes NULL to array_filter.

We already have some special handling in the Numeric field plugin around float precision when $this->definition['float'] is set, so maybe we can figure out how to use that to allow Decimal settings in aggregation output.

We also obviously need a lot more test coverage for aggregation output, some of which can be seen over in 🐛 [regression] COUNT aggregation no longer outputs count value for some fields Active

Confirmed this isn't just an issue with COUNT either, SUM etc have the same behaviour. I'm starting to think we do need to revert here and come up with a proper fix. The field handlers should be overridden because we don't want to go through the EntityField plugin when outputting aggregation data.

Test is correctly failing now:

       ├ Failed asserting that two strings are equal.
       ┊ ---·Expected
       ┊ +++·Actual
       ┊ @@ @@
       ┊ -'4'
       ┊ +'Thu,·1·Jan·1970·-·10:00\n

I'm debugging how views renders this stuff 🤯

When we get to StylePluginBase::elementPreRenderRow we have the correct count value in $data['#row']. What's happening is that value of 4 is being passed to the timestamp formatter which results in the Jan 1 1970 date (unix epoch)

Before 🐛 Decimal separator and decimals settings ignored when aggregating decimal fields Needs work , the field handler was being overridden to numeric, therefore $field in that preRender was the NumericField plugin, not EntityField.

That breaks testFieldAggregationSettings again, gonna have to dig further into this

It seems like string based fields work fine (tested the output of COUNT on the name and that works without the fixed) so perhaps it's a numeric field issue?

Tracking the regression over in 🐛 [regression] COUNT aggregation no longer outputs count value for some fields Active I've got a test case going so far.

acbramley → created an issue.

Reading through this issue a bit more, it seems like from the last screenshot in the IS the field handler should not have been removed from COUNT and COUNT DISTINCT?

This has also caused a regression for us with a pretty simply view after upgrading to 11.1.6

The view uses a COUNT aggregation on the changed field to output the number of nodes changed per month for a given date range. In 11.1.5 this output a numeric count, now it outputs the formatted changed date.

Reverting this commit fixes it again.

I've attached an example view. Should this be reverted?

Steps to reproduce:
- Install standard on 11.1.6
- Install rest module
- Import attached view
- Go to /test/count
- Notice HTML in changed_1 field
- Revert commit
- Notice counts in changed_1 field

@berdir I think that sounds good for a follow up, would you mind creating an issue?

the constants are passed to the mark template

Oh gosh I didn't realise that, I hadn't done a full grep for the constants just a CTRL+Click in PHPStorm to find usages of it.

Added a basic upgrade path, needs tests though.

Yes, but if we remove the checkboxes on new install, we do not need to worry about the checkbox being unchecked.

We aren't removing checkboxes, we're hiding them. It actually makes more sense to default to unchecked once we hide the fields, otherwise new sites will be creating "promoted" content without the checkbox even being visible. We have to do things in small steps (see the history of this issue) otherwise we'll never get anywhere :) Just look at how many tests in core assume things are promoted by default!

CR created https://www.drupal.org/node/3517642 →

We need an upgrade path here though unfortunately.

Currently if you:
1. Install Standard profile on 11.x
2. Check out this branch
3. Clear cache
4. Notice the Article node type no longer defaults to Promoted. Page correctly still defaults to Not Promoted because Standard ships with base_field_override config

Commented over there, they are 2 distinct issues.

I'm still working on the test failures.

@xmacinfo no - this is different. This is making the checkbox on the NodeType form itself to be unchecked.

A 5 digit issue almost 20 years old!

Totally agree, we can hide these by default very easily. Manually tested the MR change by:

1. Installing Standard
2. Creating a new content type and NOT changing the default form display settings
3. Making the changes in the MR
4. Clearing cache
5. Checking form display settings.

The result is as follows:
- Any content type that already had a form_display config is preserved - i.e basic page and article created by Standard
- Any content type that had no form_display config (i.e the one created in step 2 above) will have its promoted and sticky flags automatically hidden
- Any new content type created after the change will have its promoted and sticky flags automatically hidden.

I think this is expected behaviour, the content type created in 2 still had the default form_display config so updating the base field definition just updated the default display. Once the form display settings are saved at least one time, that creates the core.entity_form_display.node.test.default config entity and after that, changes to the base field definitions will not affect the form display.

Therefore, this is BC safe.

I'm not sure if this should be postponed on 📌 "Promoted to front page" for new content types should default to Un-Checked Needs work or not (hopefully not).

acbramley → made their first commit to this issue’s fork.

Found while triaging Node issues, this definitely seems like a User module thing.

I haven't read the full thread but I'm a bit confused why we'd want to show a linked username if the user doesn't have access to view the profile? We'd be showing a link to a 403?

I think the scope of this is no longer in the node system

I am not able to reproduce this issue with the steps provided. Can they be expanded to start from installing Drupal core?

Removing the constraint would mean changing live menu links when creating pending revisions. Supporting this would require a huge overhaul to the menu system to account for pending revisions. This isn't a Node issue.

See #2858434: Menu changes from node form leak into live site when creating draft revision → where this was introduced.

@nod_ what's the proposed solution then? Calling the same thing anywhere the node type's description is used?

Test manually or automatically

I meant in automated tests, i.e to catch the bug that caused this to be reverted

Whatever we decide here we should do the same for block_content as pretty sure we got copy and paste code with the same stuff.

We should probably also make the body field creation reusable in that case and use it in the NodeTypeCreationTrait

@spooky063 that's right, it's probably the most straight forward and best overall solution IMO. Nightwatch tests are already on the chopping block, and since it's only 2 tests that can both be more easily tested in PHP (WebDriver) tests it makes sense to move those anyway.

Otherwise we need to either
a) copy the same field creation code into NodeTypeForm again, we don't want to tie that form to the testing profile anyway so this wouldn't do, or
b) add more Nightwatch steps to the 2 tests, further extending the already lengthy test case. That would then need to be ported again when Nightwatch is eventually replaced.

I looked at adding default config to the testing profile but the profile is specifically very barebones so that didn't feel right either.

One of the issues is already up for review with more detail so feel free to review if you can :)

Absolutely agree with #4, I was actually trying to find which part of drupal was adding those tags we're testing in the nightwatch test. Seeing as it's the CKE5 plugin itself, we don't need the drama of testing it ourselves.

I've also added return types to 4 functions in CKEditor5TestTrait that were flagging new violations in the new test class. This means we can remove 240 lines from the baseline.

So this one is quite easy to do in PHP, except that I can't figure out how to actually type in the editor which we need to prove that adding the code block adds the specific class/markup. Without typing anything, CKE returns no data.

I need to move on for today so have pushed what I've got so far.

Postponing on converting those tests to WebDriver tests.

Created those follow ups for Nightwatch tests which are needed now for 📌 Deprecate node_add_body_field() Active

📌 Convert ckEditor5CodeSyntaxTest to WebDriver test Active
📌 Convert ckEditor5EditorHeightTest to WebDriver test Active

acbramley → created an issue.

acbramley → created an issue.

Last failure is in Nightwatch and for once doesn't look like a random fail, I'm assuming we need to add a step to it to create the body field

https://git.drupalcode.org/issue/drupal-3489266/-/jobs/4842498

Looks like only 6 tests relied on the NodeTypeForm bits https://git.drupalcode.org/issue/drupal-3489266/-/pipelines/463009/test_...

Other failures in that pipeline are random afaict

Rebased and tidied up a lot of duplication in tests by using ContentTypeCreationTrait, also added a CR.

There were 2 spots in runtime code that are still using this function:
- NodeTypeForm - only using for the testing profile, this should definitely just be removed. We don't want test code in runtime code.
- The EntityNodeType migration destination plugin - I've replaced this with the contents of node_add_body_field

So the outcome here is a little bit more duplication (between the test trait and migration plugin) but I think that's ok.

acbramley → made their first commit to this issue’s fork.

Removing the original report here as it's just confusing as it was originally the opposite.

Tests all passed with the format change which tells me we lack any test coverage for the output. Went looking and confirmed, we have CollapsedSummariesTest but that's testing view port specific things. SettingSummariesContentTypeTest is testing it on the Node type itself. I've added NodeDetailsSummariesTest which covers the author and promoted option summaries

Just ran into this in https://git.drupalcode.org/issue/drupal-3482699/-/jobs/4841663, same failure as #15 which is actually a path through several TestBase classes finally landing in OffCanvasTestTrait::waitForOffCanvasArea.

It's hard to tell if the usleep swap would fix this particular case since you'd think it'd fail earlier when calling mouseOver or click on the $block (because find would return NULL?), but it definitely makes sense regardless.

Updating tags, we'll need some steps to reproduce what sounds like a bug (should this be recategorised from Task?) and test coverage.

acbramley → created an issue.

Re-rolled a new MR against 11.x with a bunch of tidy ups. Still NW for #21

::compare requires a Collator to be passed to it as well so we'll need to allow that the handle it being NULL?

I missed the symfony polyfill in the MR though which does look like it works

This blows up sites that don't have the intl extension

adam@8fa2a3a71304:data $ drush cr
PHP Fatal error:  Uncaught Error: Class "Collator" not found in /data/app/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:239

I think the intention of this code was to check the extension was loaded before initialising the $collator but I can't quite follow:

$collator = \Collator::create((!extension_loaded('intl')) ? ('en') : (\Drupal::service('language_manager')->getCurrentLanguage()->getId()));

acbramley → made their first commit to this issue’s fork.

Adding related issue as I think replacing this variable with the view mode check would fix that issue as well? Right now we only set page to true if we're on the canonical route (or if we're in preview and also viewing the full/default view mode).

Based on #25 I think we need to understand what the expectation is here.

I've looked at the code (now lives in NodeGrantDatabaseStorage::write) and I can't see how it would "blow up". Given the age of this issue and lack of activity I'm going to close it for now.