πŸ‡¦πŸ‡ΊAustralia @acbramley

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

Merge Requests

More

Recent comments

πŸ‡¦πŸ‡ΊAustralia acbramley

Pushed changes to an MR with an upgrade path to retain BC for existing sites.

πŸ‡¦πŸ‡ΊAustralia acbramley

This should have been set to NR again with the feedback addressed all the way back in Feb last year.

We have been running this patch for years at this point. If this can't be committed yet, can we please get a detailed list of steps to get it to a point where it can be?

πŸ‡¦πŸ‡ΊAustralia acbramley

Sorry, I missed the second field. I still don't think this is something we want to or need to support.

πŸ‡¦πŸ‡ΊAustralia acbramley

This would be a good improvement, test failures are unrelated (HEAD is failing)

πŸ‡¦πŸ‡ΊAustralia acbramley

I'm wondering if we can also document when an update path test is needed?

Take πŸ› Reduce the number of field blocks created for entities (possibly to zero) Needs review for example - a critical bug that greatly affects performance of large Drupal installs.

The update hook is trivial - it sets a boolean value in a new config.

Writing an update path test for that is not trivial - even for someone extremely familiar with writing tests, and someone that has written upgrade path tests in the past (like me).

This one in particular is harder because layout_builder needs to be installed on top of the generic db dump file before we run updates, I spent ~60 minutes this morning getting this test working, and it would've been MUCH longer if I hadn't had multiple resources to help me (one being the existing MenuLinksetSettingsUpdateTest to copy from, and the layout-builder.php stub file that I stumbled upon to do the module install for me.

We then run that update hook on every single MR going forward, all to test that a one liner runs correctly? Something that any number of contributors can look at and visually confirm it's doing the right thing.

πŸ‡¦πŸ‡ΊAustralia acbramley

@dww honestly I don't think this is a Novice issue. As you have seen, it takes knowledge/investigation on what each of those comments are describing. For example, noticing the TimeInterface example could be removed entirely.

Since I've been close to all these issues recently I thought it was easier if I just did it.

πŸ‡¦πŸ‡ΊAustralia acbramley

Replaced all but one with the API equiv.

The mention in TimeInterface was removed entirely because the comment is talking about replacing things with the API call.

πŸ‡¦πŸ‡ΊAustralia acbramley

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

πŸ‡¦πŸ‡ΊAustralia acbramley

Working on an upgrade path test... bit of a waste of time if you ask me but would rather just do it to get this across the line.

πŸ‡¦πŸ‡ΊAustralia acbramley

Totally agree with #70 and #71 - why not commit as is and open a follow up to track upstream to allow enabling only one or the other?

πŸ‡¦πŸ‡ΊAustralia acbramley

I personally don't agree with this. The checkbox makes the UX confusing (you're checking a checkbox and then entering a number of days into a field that says seconds)

If this was a form that required interaction more often than once in a blue moon I think it'd make sense to improve the UX but this is just something you could calculate in the background (e.g google "5 days in seconds" and copy the value).

πŸ‡¦πŸ‡ΊAustralia acbramley

I think that bug will be fixed by πŸ“Œ Automatically set revision user/log information/created time on entity revisions Needs review but I'll need to double check.

πŸ‡¦πŸ‡ΊAustralia acbramley

@smustgrave some people use the word interchangeably with "merge"

I don't know how I messed up the merge so badly yesterday, MR was showing 220+ commits. Rebased and looking good now.

πŸ‡¦πŸ‡ΊAustralia acbramley

The MR is hitting https://www.drupal.org/project/drupal/issues/3422537 πŸ› Increment or remove GIT_DEPTH limit for GitlabCi Active and needs a clean rebase with 11.x

πŸ‡¦πŸ‡ΊAustralia acbramley

Updated field description and associated screenshot.

πŸ‡¦πŸ‡ΊAustralia acbramley

#63 is now fixed with the latest MR changes - thanks @Taran2L!

The MR is green too, should we consider unpostponing?

πŸ‡¦πŸ‡ΊAustralia acbramley

This would be great :) Thanks for all your work on the issue queue @Kristen Pol

πŸ‡¦πŸ‡ΊAustralia acbramley

Updated the CR, I think the only thing remaining from #113 is the field description.

πŸ‡¦πŸ‡ΊAustralia acbramley

See #104 re update path tests. Can add one if needed but feels like overkill.

πŸ‡¦πŸ‡ΊAustralia acbramley

See #98 for the alternative/better approach/follow up.

I will update the MR/CR tomorrow.

IMO we should not need update tests for a simple configuration set.

πŸ‡¦πŸ‡ΊAustralia acbramley

Big +1, surprised this isn't already a thing!

πŸ‡¦πŸ‡ΊAustralia acbramley

@smustgrave a conditional in the validator? Yeah that could work. However, I think we should come at these argument default plugins from a different angle and look at implementing an entity type agnostic plugin that is configurable to be entity id or revision id.

Right now we have a plugin for Node, and another plugin for Taxonomy (with some really wild config that I'd never seen before!).

Most of the time people will just want a default argument that takes the entity's ID. We should be able to derive the entity type from the route, and then we don't need N plugins for N entity types. We can also let that plugin handle revisions too.

πŸ‡¦πŸ‡ΊAustralia acbramley

πŸ“Œ Replace REQUEST_TIME in OO code w/o access to the container Needs review is the last of the issues! Once that's in I'll double check if there's any left and create a final cleanup if necessary. Great work everyone

πŸ‡¦πŸ‡ΊAustralia acbramley

Going to self RTBC since this was a simple change and we went with @mstrelan's version.

πŸ‡¦πŸ‡ΊAustralia acbramley

Feedback has been addressed, back to RTBC!

πŸ‡¦πŸ‡ΊAustralia acbramley

Overall looking really good! We have some test failures though.

πŸ‡¦πŸ‡ΊAustralia acbramley

Nice, I'm gonna mark RTBC to see if anyone thinks it should be split up :)

πŸ‡¦πŸ‡ΊAustralia acbramley

I scanned majority of the MR (up to image module). 90-95% of the changes are just test case functions. The remaining are things like setUpContentType, anonymous/inline callbacks and doAssert style functions. If it's not automatable to restrict to just test cases I'd vote for just including everything in all test files to avoid a lot of manual work.

This could be split up, but I won't comment on if it should or not :D

I also separately reviewed https://git.drupalcode.org/issue/drupal-3421418/-/compare/981d4f6368d4ff... and those changes make sense to me.

However, I'm not sure why we can't add it to EntityResourceTestBase::testPost?

For me this looks good to go, nice work @mstrelan

πŸ‡¦πŸ‡ΊAustralia acbramley

Those 2 come from traits which we're not fixing here (FakeLogEntries and AssertViewsCacheTagsTrait)

If you search the test files you'll see that there are no references to REQUEST_TIME. The traits are fixed in the functional test one I believe.

πŸ‡¦πŸ‡ΊAustralia acbramley

we can't add a node field without "field_" prefix

Yes we can, if the field_ui prefix setting is emptied which is actually quite common.

πŸ‡¦πŸ‡ΊAustralia acbramley

Peeled this back to just Kernel tests and removed the class members entirely. Used variables for $requestTime where appropriate. Pipeline is green :)

No need for a CR now since we're not adding any class members.

πŸ‡¦πŸ‡ΊAustralia acbramley

There's still a huge number of changes to non Kernel/Unit test files here. All functional/functionaljs tests are being updated in πŸ“Œ Replace REQUEST_TIME in Functional and FunctionalJavascript tests Needs work

πŸ‡¦πŸ‡ΊAustralia acbramley

acbramley β†’ changed the visibility of the branch 3309104-replace-requesttime-in to hidden.

πŸ‡¦πŸ‡ΊAustralia acbramley

Feedback addressed, pipeline's green again. Hiding the old MR.

πŸ‡¦πŸ‡ΊAustralia acbramley

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

πŸ‡¦πŸ‡ΊAustralia acbramley

Found one instance that can use DI

πŸ‡¦πŸ‡ΊAustralia acbramley

Thanks very much @catch

πŸ‡¦πŸ‡ΊAustralia acbramley

Included some phpcs fixes too.

πŸ‡¦πŸ‡ΊAustralia acbramley

I've pushed a fair chunk of work, what I've done so far:

1. Moved all logic from .module files into a new PHP class for easier reuse + dependency injection
2. Heavily refactored the above functions to be more reusable
3. Moved presave hook logic into entity_update using an entity query
4. Added new index_previous_revisions setting which is used in hook_entity_update

You can see that the full test suite still passes that I committed in πŸ“Œ Write tests Fixed - and we can remove the line that emptied search_api_revisions_update_ids.

All that's left is additional test coverage for when the new setting is off.

πŸ‡¦πŸ‡ΊAustralia acbramley

This should be done in the 2.x branch now. There's also more work to do to get this stable

πŸ‡¦πŸ‡ΊAustralia acbramley

Test coverage has been added for:

* Tracking initial revisions
* Tracking updates/inserts
* Tracking via the queue
* Tracking deletions

πŸ‡¦πŸ‡ΊAustralia acbramley

Going to add some tests today

πŸ‡¦πŸ‡ΊAustralia acbramley

Looking great, nice work

πŸ‡¦πŸ‡ΊAustralia acbramley

Big +1 to get something like this into core.

Wrt. the entity_usage β†’ module it seems there are currently 2 different rewrites in progress in the 8.x-3.x and 8.x-4.x branches so it'd be good to figure out which of those is likely to be the solution going forward.

πŸ‡¦πŸ‡ΊAustralia acbramley

Is the plan to still go ahead with this 3.x branch? I see there's now a 4.x branch using entity_track. Surely we should consolidate efforts on a single new architecture?

We use this module pretty heavily on one of our client projects and they've recently asked for features such as filtering the Usage list by current/previous revision so I'm happy to help the efforts in order to unlock so of those more complex features.

πŸ‡¦πŸ‡ΊAustralia acbramley

Triaged as part of BSI.

I tried to reproduce following the steps in the IS, however I cannot reproduce it.

I also see that the element's format is passed into text_summary in TextTrimmedFormatter

Setting to PMNMI for now, this will be closed in the future if further information is not provided.

πŸ‡¦πŸ‡ΊAustralia acbramley

But since its not improving anything why to change this, whats the reason.

Because using magic properties is not best practice. There's no reason to keep this logic in a presave hook when we can combine it with the update hook and make it much cleaner.

Why you would like to add setting not to index all revisions since module itself doing those flags which needs to be updated

In our use case, we are not indexing any of these flags. We simply need to index basic information across many different entity types. This information does not change once a revision is saved, therefore reindexing every revision is redundant.

e can of course add this settings thats easy but not sure how to deactivate those flags if this is turned off, i dont want setting that causes wrong flags

It would just be another setting like the queue setting. We can have it enabled by default to retain the current behaviour, but allow users (like me) to disable it :)

πŸ‡¦πŸ‡ΊAustralia acbramley

acbramley β†’ changed the visibility of the branch 4.0.x to hidden.

πŸ‡¦πŸ‡ΊAustralia acbramley

acbramley β†’ changed the visibility of the branch 3403538-dont-trigger-expiration to active.

πŸ‡¦πŸ‡ΊAustralia acbramley

@mkolar ok that does make sense.

I don't think we need to do this logic in a presave though? We can do it directly in the entity_update hook and do away with the magic search_api_revisions_update_ids property. In entity_update we can just select all revisions that aren't the current revision. We can also add a setting for this logic alongside the queue setting.

πŸ‡¦πŸ‡ΊAustralia acbramley

Turns out that the fields reindexing was due to a separate unrelated bug with the entity type I was testing.

πŸ‡¦πŸ‡ΊAustralia acbramley

@alexpott that's a shame :(

but the amount of memory you are using points to something going wrong

Do you have any ideas of threads I could pull on? Pretty keen to get to the bottom of it!

πŸ‡¦πŸ‡ΊAustralia acbramley

Hiding patches to reduce confusion.

FYI - you can download an MR diff locally and use that in your composer workflow instead of uploading to an issue. e.g https://git.drupalcode.org/project/drupal/-/merge_requests/3094.diff

πŸ‡¦πŸ‡ΊAustralia acbramley

Looking good, thanks!

Re #14 - yes that can definitely be tidied up in another issue :)

πŸ‡¦πŸ‡ΊAustralia acbramley

@hyperlinked fwiw, you should always run updb before config import. Database updates can make changes to your configuration, those should be run on a local/development environment first and then the config changes should be exported. Then when deployed you'll have a clean deploy process with updb then import. This is how drush deploy works (highly recommend using that :))

πŸ‡¦πŸ‡ΊAustralia acbramley

PHPCS is now failing... I don't understand why those variables had to change. They did not need to be fixed, proven by a green pipeline and my manual testing :)

πŸ‡¦πŸ‡ΊAustralia acbramley

This is good to go again

πŸ‡¦πŸ‡ΊAustralia acbramley

acbramley β†’ changed the visibility of the branch 3419356-use-constructor-property to hidden.

πŸ‡¦πŸ‡ΊAustralia acbramley

CRs for this issue are also still in draft (maybe that's right?) but do need version updates.

It also may be wise to combine the CRs around the new dependencies once 3113971 is in?

πŸ‡¦πŸ‡ΊAustralia acbramley

This issue conflicts heavily with πŸ“Œ Replace REQUEST_TIME in services Needs work - one thing we did there was handle string typed $max_rows in DatabaseBackend which this issue didn't handle. That could lead to a BC break.

See https://git.drupalcode.org/project/drupal/-/merge_requests/4302#note_259560

πŸ‡¦πŸ‡ΊAustralia acbramley

Thanks @catch - my hope was to avoid more round trips and nasty conflicts again but this has bitten me badly now and we have some horrendous stuff to deal with now that ✨ Make serializer customizable for Cache\DatabaseBackend RTBC is in. I'm not even quite sure how to handle the now 2 new optional params in DatabaseBackend. That issue didn't take into account string $max_rows either (discussed in the MR on this issue).

πŸ‡¦πŸ‡ΊAustralia acbramley

Feedback addressed, thanks for the thorough review @longwave

Created follow-up in ✨ Add translation revert functionality for generic revision UI Active

πŸ‡¦πŸ‡ΊAustralia acbramley

Adding related revisions UI issue.

πŸ‡¦πŸ‡ΊAustralia acbramley

Pushed test coverage for #81, still unsure how to reproduce it via the UI since Media goes through that same form base.

The !$this->getRevisionUser() check in ContentEntityBase also fails on API based updates so we may need to remove that and always hardset to the current user.

πŸ‡¦πŸ‡ΊAustralia acbramley

To answer #82 - it comes from here https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

Manually testing Media works fine for me.

However, it doesn't make sense to set the revision author in 2 places, and it especially doesn't make sense to set it to the author. I think we should remove it from the form and fix the entity base implementation.

πŸ‡¦πŸ‡ΊAustralia acbramley

NW based on #81 - great find @Tomefa

The weird part is, that code looks to have come from Node (setting the revision author to the author if it's not set) so I wonder why that same issue doesn't already apply to Node.

Will try to dig into it today.

Marking as Needs tests to cover that scenario (editing with a different user)

πŸ‡¦πŸ‡ΊAustralia acbramley

Already discussed this in slack and this is looking great, test coverage is perfect! Thanks @dww

πŸ‡¦πŸ‡ΊAustralia acbramley

Marked πŸ› big_pipe JS checkMutation can throw type errors if parentNode is NULL Closed: duplicate as a duplicate. This still needs tests. Please keep work in the MR, patches are not required here.

Production build https://api.contrib.social 0.61.6-2-g546bc20