πŸ‡¦πŸ‡ΊAustralia @acbramley

Account created on 22 November 2010, over 13 years ago
#

Merge Requests

More

Recent comments

πŸ‡¦πŸ‡ΊAustralia acbramley

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 :)

πŸ‡¦πŸ‡ΊAustralia acbramley

Looks like this is still missing an IS update with clear steps on how to reproduce. I don't quite follow what #25 and #26 are showing.

πŸ‡¦πŸ‡ΊAustralia acbramley

This was triaged as part of BSI. At the very least needs a reroll onto an MR.

πŸ‡¦πŸ‡ΊAustralia acbramley

but the CR stops me from doing that

What do you mean by this?

πŸ‡¦πŸ‡ΊAustralia acbramley

PHPstan is failing. You can add the error to the ignore list in the phpstan.neon file.

πŸ‡¦πŸ‡ΊAustralia acbramley

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.

πŸ‡¦πŸ‡ΊAustralia acbramley

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.

πŸ‡¦πŸ‡ΊAustralia acbramley

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!

πŸ‡¦πŸ‡ΊAustralia acbramley

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.

πŸ‡¦πŸ‡ΊAustralia acbramley

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.

πŸ‡¦πŸ‡ΊAustralia acbramley

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.

πŸ‡¦πŸ‡ΊAustralia acbramley

@COBadger this issue is Closed, not sure why you're pushing to an MR?

πŸ‡¦πŸ‡ΊAustralia acbramley

@quietone I checked the documentation and made a minor change to add indentation to the enum example. Otherwise looks good to me!

πŸ‡¦πŸ‡ΊAustralia acbramley

Added indentation to the Enum example.

πŸ‡¦πŸ‡ΊAustralia acbramley

πŸ› 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?

πŸ‡¦πŸ‡ΊAustralia acbramley

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.

πŸ‡¦πŸ‡ΊAustralia acbramley

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

πŸ‡¦πŸ‡ΊAustralia acbramley

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.

πŸ‡¦πŸ‡ΊAustralia acbramley

This is almost 6 years old with no additional comments, is this still a valid bug on 11.x?

πŸ‡¦πŸ‡ΊAustralia acbramley

Is there a plan to drop this entirely? E.g in a 2.x branch? Could also drop Drupal 9 support in there ;)

πŸ‡¦πŸ‡ΊAustralia acbramley

Reset the branch back to before the experimentation for removing dupes.

πŸ‡¦πŸ‡ΊAustralia acbramley

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?

πŸ‡¦πŸ‡ΊAustralia acbramley

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.

πŸ‡¦πŸ‡ΊAustralia acbramley

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

πŸ‡¦πŸ‡ΊAustralia acbramley

Thank you for your contribution. This issue currently does not meet the Contribution guidelines which are required to get this change committed.

πŸ‡¦πŸ‡ΊAustralia acbramley

Adding related issue, we're running into duplicate issues there too so maybe we can share some ideas between the 2.

πŸ‡¦πŸ‡ΊAustralia acbramley

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.

πŸ‡¦πŸ‡ΊAustralia acbramley

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.

πŸ‡¦πŸ‡ΊAustralia acbramley

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...

πŸ‡¦πŸ‡ΊAustralia acbramley

Spoke to @catch in slack yesterday and going to pull on the thread of #110

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 (because the plugin ID is stored in config), and then when they're saved again, it would update to the entity reference plugin instead.
And if this is the case it might be fine.

This is to avoid having a new field.

πŸ‡¦πŸ‡ΊAustralia acbramley

Re #94 - I think (at least this is a limitation of Hux) that no themeing hooks work at all, including hook_theme. Would be good to confirm that too.

πŸ‡¦πŸ‡ΊAustralia acbramley

I'm interested in helping to move this issue along. Given #17, should we just get the PoC MR green? The few examples there cover a decent number of scenarios.

πŸ‡¦πŸ‡ΊAustralia acbramley

Back to NR. We should probably squash all commits on the branch back to a single commit if this goes green too.

πŸ‡¦πŸ‡ΊAustralia acbramley

@webdrips no, because it confuses the issues and makes things much harder to manage for reviewers and commiters. This issue is already extremely long in the tooth (almost at 200 comments and we've already had to recreate the issue because the last one had too many comments to load). Uploading patches will only cause more issues.

πŸ‡¦πŸ‡ΊAustralia acbramley

@abx don't use MR .diff URLs, that is opening your site up to security vulnerabilities since anyone can push to that branch (and then subsequently make changes to the module when your site installs the new patch).

Instead you should download and store the diff locally.

πŸ‡¦πŸ‡ΊAustralia acbramley

Pushed a new fix which uses a yaml template instead, this gets around the issues with escaping the colon in the sed command.

πŸ‡¦πŸ‡ΊAustralia acbramley

@djg_tram I actually just noticed the same error on a fresh Standard install of Drupal with Layout builder disabled. This warning is not related to this issue.

πŸ‡¦πŸ‡ΊAustralia acbramley

@djg_tram thank you for the message, this was covered in the CR for the original issue that changed this functionality https://www.drupal.org/node/3416592 β†’

This CR was outdated in that it was still referring to the setting that was originally developed. However, in your case, re-enabling the module will fix your issues once caches are cleared. I have now fixed this.

If you're still having issues, please open a bug report.

Thanks!

πŸ‡¦πŸ‡ΊAustralia acbramley

@jonathan1055 script output changes look great, thanks!

πŸ‡¦πŸ‡ΊAustralia acbramley

@jonathan1055 thanks, that wasn't obvious from the job output. Still learning my way round the repo

πŸ‡¦πŸ‡ΊAustralia acbramley

WIP pushed, seeing some weirdness with revision times once they're processed but so far it's functional.

πŸ‡¦πŸ‡ΊAustralia acbramley

acbramley β†’ created an issue.

πŸ‡¦πŸ‡ΊAustralia acbramley

Rerolled against 3.0.x in a new MR.

πŸ‡¦πŸ‡ΊAustralia acbramley

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

πŸ‡¦πŸ‡ΊAustralia acbramley

Bump, 10.3 is out now. The pipeline is green, we should also turn off Drupal CI.

πŸ‡¦πŸ‡ΊAustralia acbramley

Tested the sed on the CLI and it seemed to work nicely, my IDE was giving me strange syntax highlighting so may need some escaping around the colon, but hopefully you get the idea :)

πŸ‡¦πŸ‡ΊAustralia acbramley

More issues with the current script:

If you have -core_version_requirement: ^10.3

You'll get core_version_requirement: ^10 || ^11.3

πŸ‡¦πŸ‡ΊAustralia acbramley

So NEXT_MAJOR wasn't running because of our core_version_requirement, see the grep/sed in the log output:

$ grep -q "\^11" *.info.yml || (grep -q "\^10" *.info.yml && sed -i "s/\^10/\^10 \|\| ^11/" *.info.yml)

I don't think it's safe to say '>=10.3' - we should be defining this more strictly (and following what the general case is in the community with defining specific core versions.

πŸ‡¦πŸ‡ΊAustralia acbramley

Proven in https://git.drupalcode.org/issue/scheduled_transitions-3456090/-/jobs/19... the 11.x deploystack is running on 10.3.0

However, this new method isn't running any tests on 11.x....

πŸ‡¦πŸ‡ΊAustralia acbramley

Suggest doing something like this instead in gitlab CI:


variables:
  _PHPUNIT_CONCURRENT: 1
  OPT_IN_TEST_NEXT_MINOR: 1
  OPT_IN_TEST_NEXT_MAJOR: 1

phpunit (next major):
  variables:
    SYMFONY_DEPRECATIONS_HELPER: "disabled"

phpstan (next major):
  allow_failure: true

phpstan (next minor):
  allow_failure: true
πŸ‡¦πŸ‡ΊAustralia acbramley

Forgot to reply to this:

Essentially this is a more complex variant of unlocked slots (which you mentioned above). I'm not at all certain it is a good idea! It was discussed at some point (maybe even just at DrupalCon β€” I do not recall). It kind of falls under the " I'm not saying I think this is a good idea. This is more on the extreme end of granularity of control." caveat I wrote in #2.

Which was a reply to @catch's question about unlocking only certain slots in #18

As per above in #26 I think this kind of flexibility is a good idea and somewhat required. In my example above we could imagine that on the Basic page maybe the Sidebar region (or slot?) of the 2 col 80/20 layout may be locked. E.g I can imagine a use case where I want CC to be able to edit the main region of that 2 col layout, but I want to lock the sidebar to only ever display the right hand tree navigation component, and nothing else.

πŸ‡¦πŸ‡ΊAustralia acbramley

What does this mean? The per-view mode default layout?

I mean layout plugins, usually defined in our theme (or modules) foo.layouts.yml

Do you have specific example scenarios where this is needed that you could walk us through?

Definitely, we use Layout Builder Restrictions β†’ on basically every project that uses LB - it's essentially a mandatory module IMO if you're allowing layout overrides.

One way we use it is something like this:

Say we have 2 bundles - Basic Page (BP) and Landing Page (LP).

We have 3 Layouts defined in our theme - Sidebar , Cards, and Edge.
Sidebar is an 80/20 split layout with regions "main" and "aside"

Cards is a variable layout that allows some full width content with a variable number of columns of "cards" underneath (see Lee's blog post for more info on this) with regions "intro" and "cards"

Edge is a full bleed layout that expands across the whole page with only 1 region "main"

With LB Restrictions I can setup the following config:

On BP content
Only allow the Sidebar layout, do not allow Cards or Edge.
On the Sidebar layout, only allow certain components in the main region and certain components in the aside. E.g I do not want CCs adding my right hand menu to the main region, and I don't want image cards in the aside.

On LP content
Allow all layouts
Apply the same rules as BP content to Sidebar layouts, and maybe allow an additional component (because it should allow different permutations per bundle)
On Edge layouts only allow a "Hero" component - taking it a step further it could be ideal to restrict this to only a single Hero component and nothing else (this extra bit currently isn't possible with LB restrictions afaik)
On Cards layouts only allow simple Text components in the Intro region, and Card components in the Cards region.

πŸ‡¦πŸ‡ΊAustralia acbramley

@deviantintegral thanks very much for the reply!

πŸ‡¦πŸ‡ΊAustralia acbramley

This is great, thank you so much Wim. I've read through all notes so far and it's cleared up quite a few questions I had.

On the note of CC freedom, I think one thing that would be ideal (and I'm pretty sure is covered but there is a bit of nuance) is having the ability to be able to configure the amount of freedom per region per layout per bundle. As in at level in bold, as a site builder, I want to be able to configure the "freedom" of the CC.

This is essentially what Layout builder restrictions (and lock I believe) modules allow you to do. I know those layers are named differently I'm XB but I believe there's a similar concept (although XB allows more nesting).

Ideally, also, let's try and make the UI for configuring this good for the site builder πŸ˜…

πŸ‡¦πŸ‡ΊAustralia acbramley

again, I linked the issue where the comment was posted above.

Sorry, I saw you link the issue but didn't (and still don't) see that was where the comments were coming from. Should've checked it I guess.

πŸ‡¦πŸ‡ΊAustralia acbramley

Checked the methods and yes this still applies to phpunit, as per #32 and #5 we should throw an exception in createRole instead of returning FALSE.

We should start with a reroll of #5 on an MR against 11.x

πŸ‡¦πŸ‡ΊAustralia acbramley

Re #218 where are these comments coming from? The form is definitely themed as a frontend page (i.e the site's themed). Reworded the release note to remove references to this and provide better guidance. I was trying to keep the note brief as more information is available in the CR (which I linked to).

Production build 0.69.0 2024