localgovdrupal / localgov_events

Events for LocalGov Drupal.
GNU General Public License v2.0
1 stars 0 forks source link

Fix #17 - on first import there event doesn't exist yet. #18

Closed ekes closed 2 years ago

ekes commented 3 years ago

This is when the module is being installed by a configuration import that has got the fields attached anyway (or not as the case is).

It shouldn't effect the behaviour, tested, of the installation when enabling the module for the first time.

finnlewis commented 3 years ago

This looks trivial enough to me and fixes the install problem. I'll leave it a day or so though in case anyone else wants to review. @andybroomfield @Adnan-cds @stephen-cox ?

finnlewis commented 3 years ago

Once this is approved, I think we'd like to tag another release to assist with our deploy on Thursday.

Adnan-cds commented 3 years ago

Sorry Finn, I will leave this one for Andy to review.

FAO @andybroomfield

andybroomfield commented 3 years ago

Not sure what I'm reviewing @ekes @finnlewis. Is it installing by reimport config or using config installer?

andybroomfield commented 3 years ago

Is this issue #17 or issue #17 and #15 that this is meant to address?

Since I can't replicate the issue (on fresh installs), i'd be hesitant about simply returning if the form_display or view_display are empty, as that sounds important.

finnlewis commented 3 years ago

Thanks for looking at this @andybroomfield.

You are right to point out that directories needs to be installed. On Lambeth we had directories installed first, then tried to enable events, generating the error.

Just testing again locally but so far I am also finding it hard to replicate the error on #17 AssertionError: assert($form_display instanceof EntityFormDisplayInterface) line 64 localgov_events.module #17

@ekes do you have a qay to reliably reproduce the error?

ekes commented 3 years ago

Assuming it's still the case this is an issue: to replicate an assertion error you will have to have assertion errors on. Generally they are in debug environments only - which is why they are nice highlighting a failed assumption https://www.php.net/manual/en/function.assert.php https://www.php.net/manual/en/function.assert-options.php

But then if it wasn't failing at the assertion it should be operating on null here https://github.com/localgovdrupal/localgov_events/pull/18/files#R107 and or here https://github.com/localgovdrupal/localgov_events/pull/18/files#R121 Which generally also throws an exception these days. So I assume this will require work to check what the sequence was.

finnlewis commented 2 years ago

@andybroomfield @ekes any thoughts on whether we want to keep this one alive ?

ekes commented 2 years ago

Yes, if it was an issue we should review this pattern - that we use quite often. Although the implications of not reacting on is_sync'ing presently hurts my head.

finnlewis commented 2 years ago

Just discussing this in the Tech Drop-in. Agreed to try to replicate the issue before confirming if this change fixes it.

andybroomfield commented 2 years ago

Closed in favour of #73