I've rolled ##24 onto an MR and updated the event dispatcher based on https://www.drupal.org/node/3376090 →
This was set to NW all the way back in #7 so I think this may be ready for another review.
The main thing I'm interested in is allowing menu links to have their cacheable metadata refined (i.e adding RefinableCacheableDependencyTrait to MenuLinkBase).
acbramley → made their first commit to this issue’s fork.
Now that I'm actually manually testing the scenario described in the IS I don't think this is a valid bug anymore (see my comment on the MR).
Perhaps at the time this issue was created, we didn't call $this->fail() inside checkPermissions OR as per #32 phpunit halts execution on a fail.
Since we fail when there's an invalid permission, the original issue in the IS does not happen because drupalLogin is never called when an invalid permission is passed through.
Settintg to PMNMI to see if there's other scenarios that we need to account for.
This will need to be rolled into an MR.
Only 2 services are added here, the rest are all autoconfigurable/autowireable but that can be done elsewhere.
acbramley → created an issue.
@mandclu I've never used this module but I maintain the Diff module. I had similar issue with another module so I've created issues for all modules extending this class.
You'd just need to install Diff 2.x and diff something using this module's field type.
acbramley → created an issue.
acbramley → created an issue.
acbramley → created an issue.
acbramley → created an issue.
Pushed to an MR.
This will fix 2.x support and not break 8.x-1.x support (since adding return types on child classes does not break compatibility).
acbramley → made their first commit to this issue’s fork.
Pushed to an MR.
This will fix 2.x support and not break 8.x-1.x support (since adding return types on child classes does not break compatibility).
acbramley → made their first commit to this issue’s fork.
This definitely does not happen on any diff.
This function is called by the caxy/php-htmldiff library, not the diff module directly.
I will need steps to reproduce with sample content.
Copied the test from #2 and applied it to the MR, this failed locally so I believe this is still valid in some respect although I'm not enitrely sure this would happen in a real world scenario?
acbramley → made their first commit to this issue’s fork.
Thanks for reporting this issue. We rely on issue reports like this one to resolve bugs and improve Drupal core.
As part of the Bug Smash Initiative, we are triaging issues that are marked "Postponed (maintainer needs more info)". This issue was marked "Postponed (maintainer needs more info)" more than 1 year ago.
Since we need more information to move forward with this issue, I am closing it.
Please feel free to reopen with more information.
This was triaged as part of Bug Smash Initiative.
I tried reproducing this on 10.3 but couldn't:
\Drupal::entityTypeManager()->getStorage('media_type')
->getQuery()
->condition('name', ['Document'], 'IN')
->accessCheck(FALSE)->execute();
I'm tempted to close this due to the lack of comments and updates since the last time this was traiged, but we will leave it in PMNMI.
Cheers.
Manually testing #46 and it does seem like this is still an issue, however I have no idea why this is a bug (as in how people encounter it or if it's a real issue?) and how to trigger it without manually visiting the URL.
All version comparison code is in drupal core
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/syste...
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/syste...
[#3450770] great catch!
acbramley → made their first commit to this issue’s fork.
I cannot reproduce this on 8.3 with Diff 2.x.
Can you please try to reproduce this with a vanilla Drupal installation.
@solideogloria that's a great idea!
As for knowing whether you can disable it (without the great suggestion in #17) you could check if any of your view_display config that uses layout builder contains either field_block:ENTITY_TYPE_ID or extra_field_block:ENTITY_TYPE_ID blocks, where ENTITY_TYPE_ID is the machine name of an entity type that does NOT use layout builder.
If there are no field blocks in use, or only field blocks in use which have at least one bundle that uses layout builder, you can safely disable the module.
I'm trying to reproduce this in a test but I'm not able to. This popped up for us during a migration. Running the rebuild tree drush command has fixed it though so I'm a bit confused if/how to get it failing in a test.
Encountered this too, the order of the children rows does not match their weight.
The children form does have a #weight element, but it's set to the delta from the children. The order of the children from $storage->findChildren is based on the left_pos, not the weight.
Even if the #weight element was set to the correct weight, the elements still need sorting (since findChildren does not return items in the correct order).
I will work on a failing test for this.
Looks great, thanks @DYdave
Added an easily copyable comment for closing long standing PMNMI issues
At least needs a reroll, a similar issue popped up today in slack ✨ Xss::filterAdmin() to allow "dialog" Needs review - wondering what the use-case is for adding data elements?
The setting is both in default config (config/install/diff.settings) and was set by an update hook.
The only way I could see to trigger this error is to not run an update hook after upgrading.
I will need steps to reproduce. Fixes also must be in an MR.
See related core issue regarding the generic revision UI - we need to defend against empty timestamps 🐛 Protect against empty revision timestamps in VersionHistoryController::getRevisionDescription Active
Thanks for reporting this issue. We rely on issue reports like this one to resolve bugs and improve Drupal core.
As part of the Bug Smash Initiative, we are triaging issues that are marked "Postponed (maintainer needs more info)". This issue was marked "Postponed (maintainer needs more info)" more than 1 year ago.
Since we need more information to move forward with this issue, I am closing it.
Please feel free to reopen with more information.
As per #47 - needs to go to an MR.
4 years on, is this still on the cards?
Looks like this is still valid (install hook in shortcut on 11.x still looks roughly the same), needs a reroll into an MR.
Quoting @catch from Slack:
I think we need to try to summarize some of the different models in that issue in the issue summary with pros and cons as well as documenting the use cases a bit better.
Big +1 for that :)
This was triaged as part of BSI. At the very least needs a reroll onto an MR.
griffynh → credited acbramley → .
@bhogue did you check the linked issue from #27? It's been fixed :)
but the CR stops me from doing that
What do you mean by this?
This is available in 2.0.0-beta2
Perfect, thank you!
PHPstan is failing. You can add the error to the ignore list in the phpstan.neon file.
Thanks for the report, yes we can remove the final, I had no idea that module was extending it, sorry!
You'll need to roll the patch into an MR. Patches are no longer supported.
acbramley → created an issue.
Cannot be tested as per #12 I also think this would fall into the bucket of a trivial fix that would be too much effort to test.
Re the rest of #12 - yes it's sort of bogus, but it is used in places to determine if it was a new or existing entity (as per the doc block).
Looking through other parts of core, there are many that are missing return values though, so just fixing NodeForm isn't really going to help us much (See CommentForm, CommentTypeForm, ActionFormBase, etc)
So I agree, we should instead deprecate the return value and remove it entirely at some point.
Nice!
acbramley → created an issue.
This issue was triaged as part of the Bug smash initiative.
I'm setting this to Postponed as it has been some time since the last comment, and there are no clear steps to reproduce the issue.
So first off, is this still reproducible on 10.3 or 11.x from a fresh install of Drupal? If so, please update the issue summary with those steps.
Tagging with an IS update too as it is lacking detail.
Once we confirm the issue is still present, we'll need a reroll on an MR against 11.x.
Thanks!
Great job guys, this is a good one to get in!
IMO one of the best ways to solve this would be to stop using yaml files and use the entity API instead to create views in the tests themselves.
This is obviously tricky because views entities don't have an API so it'll be a lot of ArrayPI type things, BUT it will help with an issue I think we're probably running into over years of core dev: No one is really sure what each test view is, what it was created for, and what it should be used for (without going through git). This is resulting in many test views being created or abused in a way just to make testing easier.
I.e - I guarantee many test views are just copy pasted with some small thing changed (add a filter, change a plugin, etc) without actually thinking about if all the things in the view are required for that new test (I know, because I've done exactly this before).
If instead the test itself documented what it needed (in the form of creating the view in the test) then it would be much clearer what bits are actually needed for that particular test.
CR reads well. Nice!
Looks good!
@jordan.jamous it is in 10.3.0.
No, that is in a different part of the codebase.
This issue was triaged as part of the Bug smash initiative.
This seems more like a support request, there doesn't seem to be any particular bug pointed out (manage display certainly works), so closing as cannot reproduce.
If this is reproducible, please re-open with detailed steps to reproduce from a fresh install of Drupal core.
I've created 🐛 Protect against empty revision timestamps in VersionHistoryController::getRevisionDescription Active as I think we should be defensive for empty timestamps regardless of default values.
acbramley → created an issue.
acbramley → created an issue.
@COBadger this issue is Closed, not sure why you're pushing to an MR?
@quietone I checked the documentation and made a minor change to add indentation to the enum example. Otherwise looks good to me!
🐛 Translated field definition are not available as source field Fixed contained a fix to a similar place in code but didn't contain tests, perhaps this could be committed too?
I've created an MR with a new approach which is to use the $bundle as part of the query condition, saving us any further processing.
Still needs tests.
Hiding patches as they are no longer supported for testing, also added gitlab-ci.yml to run tests.
acbramley → made their first commit to this issue’s fork.
Yup this is a dupe of 🐛 Not all fields are being mapped Needs work
Agree somewhat with #19 (although there definitely are uses for migrate in the current database). Seems very odd not to support D10 -> D10 migrations OOTB.
This is almost 6 years old with no additional comments, is this still a valid bug on 11.x?
Is there a plan to drop this entirely? E.g in a 2.x branch? Could also drop Drupal 9 support in there ;)
Reset the branch back to before the experimentation for removing dupes.
The question is, is there a real risk we break things in contrib here that do something similar to what the tests are doing
I think that's definitely a concern, as well as custom sites overriding plugins in data_alter hooks without updating their config... How would we deal with that?
🐛 Media author views filter should use the user_name plugin Needs work is another similar one
Contains a bit more than just the module merge, but since this is on a new branch I wanted to get a few extra things in.
griffynh → credited acbramley → .
acbramley → made their first commit to this issue’s fork.
Thank you for your contribution. This issue currently does not meet the Contribution guidelines which are required to get this change committed.
Adding related issue, we're running into duplicate issues there too so maybe we can share some ideas between the 2.
If we swap instead of duplicating, and don't provide a config upgrade path, what I think would happen is that existing views would continue to use the numeric plugin
Unfortunately this is not the case.
After a lot of experimenting, I finally found a base field to test this properly on - you need a base field that's a content entity reference that's not the author (because that already uses another plugin). What I used was the Comment Parent ID.
Test steps:
1. Install standard profile
2. Edit the content view and add a relationship to the Comments field on the node
3. Add an exposed filter on Comment: Parent CID using the above relationship. This will be a numeric filter
4. Add some comments to a node and test that the filter works
5. Edit EntityViewsData::processViewsDataForEntityReference and change $views_field['filter']['id'] = 'numeric'; to $views_field['filter']['id'] = 'entity_reference';
6. Clear cache
7. Test the filter works again.
You're presented with the following error:
Very surprised that the plugin is evaluated at runtime but so be it.
This is now a tricky situation, because I agree (after doing some more testing) that the current solution is not something we should commit. You currently get duplicate filters for every single entity reference filter. This includes fields like Authored By, which already have special filters (e.g for Authored by, it uses the user_name filter plugin)
But swapping out numeric filters may cause other issues for existing sites.
Not really sure where to go from here, so I think this needs to be Postponed until we come up with a viable solution.
This will most likely be my last push on this issue. Someone else will have to pick up the torch from here on out as I've lost interest in getting it across the line due to all the rework. Unfortunately that probably means this issue is dead in the water.
I think we're going to have quite a few issues with this new approach:
1. The update path test will probably fail because the author will be set by the preSave
2. It's impossible to test that preSave because the author will be set for new blocks by the default callback
3. It's really ugly IMO.
NULL seems OK if we could do it, but not sure what the implications of that are.
Doing some testing locally - I don't see any issues with leaving it NULL, the owner field is already nullable and the default value is null. That means when we install the new field (in block_content_update_10301) all existing rows get NULL as the uid column value. So step 1 is taken care of automatically :)
Next step was to test what currently happens with existing blocks if you edit them without an author, I expected the defaultValueCallback from EntityOwnerTrait to kick in and set the owner to the current user, but that does not happen on update, only on insert. IMO that's good - one less thing for us to worry about overriding.
(FYI That does not happen for revision_user - I reported this somewhere else but forgot about it, Block Content's revision_user is not set on the initial revision only on subsequent revisions. Node acts correctly so there must be some custom code doing that rather than using the default value callback. Tested MenuLinkContent as well and that has the same issue. I think
📌
Automatically set revision user/log information/created time on entity revisions
Needs review
will cover this)
Next is to implement a preSave which sets the owner based on the initial revision's revision_user (which may be empty for some blocks because of the above, not much we can do about that though). As I'm writing this though I realise this can't use the same database queries though...