hissy / addon_multiple_express_entry_selector

Add a new attribute type to select one or more express entries
2 stars 2 forks source link

Using Single and Multi Express Entry Selector Attributes on same Express Object, breaks Single Express Entry Selector #4

Open BloodyIron opened 3 years ago

BloodyIron commented 3 years ago

This is effectively a bug report for -> https://github.com/concrete5/concrete5/issues/5718#issuecomment-700385266

Copied text from that:

So I've tried using the Express Multi-Select, and in my dev testing (concrete5 8.5.4) When I add the multi-select addon manually that @hissy developed, I cannot use Single and Multi-Select attributes on the same Express Object, which is a problem for me.

Example scenario:

First Object, is an Event Location (hotel)

Second Object is an Event Location set of Sub-Sections (multiple meeting rooms, halls, etc)

Second Object is Events

Naturally, the Event Location associates with multiple Sub-Locations in a One-to-Many relationship. That works.

Then I create an Event Location, and two Event Location Sub-Sections. Associate the two Sub-Sections with the test Event Location, and that works.

Then I create the Event Object in such a way that it has two associations: -Event Object associated with Event Location Object, in (Event) Many-To-One (Location) relationship -Event Object associated with Event Location Sub-Sections (Event) Many-To-Many (Location Sub-Sections) relationship

Then when I define the Event Object I have Name Attribute, Single Express Select (Location) and Multi Express Select (Location Sub-Sections).

I then make a form with Name Attribute, Single Express Select, and Multi-Express Select.

I can enter the Name for the event, but Single Express Select presents NO options (when I know a location exists) BUT Multi-Express Select does work.

I think the issue is when using a Single Express Select AND Multi-Express Select on the same object, disabling the Single Express Select (and I really do need both to work for many reasons!).

Sorry, bit of a word-ful explanation :/

dimger commented 3 years ago

see #5 and #6

BloodyIron commented 3 years ago

@dimger so am I reading this right, the fix for this is already mainlined and I should update the addon? :D!

Thanks for your help! But yeah, I'd like to know if I'm reading this right.

dimger commented 3 years ago

@BloodyIron yes the master branch has now the fix.

BloodyIron commented 3 years ago

Hey @dimger so I replaced the addon with the updated master code, and I'm still having the same issue. As a heads-up, I'm using the dashboard to create the new entry for the Express Object, I'm not yet using a form on a web page. Can you please check again on your end? Thanks! :D

BloodyIron commented 3 years ago

@dimger @hissy I'm still stuck with this not working :( really would appreciate help sorting out the dashboard form issue. I haven't yet tested if the regular page forms is fixed or not as I'm still doing structural testing first.

mnakalay commented 3 years ago

I posted a solution for you in slack @BloodyIron and the problem is purely from the core attribute not from this multi-select attribute

BloodyIron commented 3 years ago

@mnakalay Here's a screenshot of the dashboard create new entry form (part of it anyways) : https://i.imgur.com/a11pJvf.png

"Which Event Location" is supposed to be a pull-down select menu where I select one Express Entry from the Event Locations Express Object, that's a (Event Locations) Many to One (Events) relationship. And you can see the select menu doesn't exist.

Meanwhile the "Event Locations Sub-Sections..." is a Multi-Express Select which does work, for a completely different Express Object.

If this is in-fact related to the core attribute, as you say (which I don't entirely understand), I'm not sure what I can do about this, as this behaviour does not meet expectation.

I do appreciate your help with this!

mnakalay commented 3 years ago

In the coreexpress attribute (single selector) form.php You have this:

if (isset($entity, $entry) && $entity instanceof Entity && $entry instanceof Entry) {
    echo $entrySelector->selectEntry($entity, $this->field('value'), $entry);
}

The problem is when you didn't select anything yet $entry is null and so the selector is not displayed... Silly.

Replace it with this and it will work:

if (isset($entity) && $entity instanceof Entity && (is_null($entry) || $entry instanceof Entry)) {
    echo $entrySelector->selectEntry($entity, $this->field('value'), $entry);
}

Now as you know modifying core stuff is not recommended and overriding express is a pain in the rompus so I strongly suggest you create a new attribute type extending the existing one, use the modified form, and then use the new attribute type instead of the core one.

As an FYI and since you mentioned v9, they fixed it by making it even simpler like this:

if (isset($entity) && $entity instanceof Entity) {
    echo $entrySelector->selectEntry($entity, $this->field('value'), $entry);
}
mnakalay commented 3 years ago

I'm not sure what I can do about this, as this behaviour does not meet expectation.

Are the code and explanation above helping?

BloodyIron commented 3 years ago

I'm going to have to try that when I get a chance, thanks! Probably in the next day or so :)

BloodyIron commented 3 years ago

@mnakalay I am having a pig of a time finding which form file I need to modify here. Also please note that I have upgraded this dev system to 8.5.4 by putting the update in ./updates/concrete5-8.5.4 but I've checked there.

I keep finding form.php files that are barely populated, or look to be irrelevant. Would you mind being explicit as to where I should be looking for this? Also, thanks for your help! I hope this solves this :)

BloodyIron commented 3 years ago

/updates/concrete5-8.5.4/concrete/attributes/express/

The form.php in there is where I needed to modify it, for others that need to know!