-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added check for field_ui module enabled durning getting field prefix #988
base: 3.x
Are you sure you want to change the base?
Conversation
This will prevent future errors related to #983, but I can imagine it breaking a lot of existing sites. Anyone who currently has field_ui disabled, quite a common setting on productions instances, will find that all their fields will lose data as they are now looking for different attribute name on apigee. |
@audave, I agree to what you said, |
The problem is not so much what the prefix is but that it changes depending on the state of a different module. In my opinion there is no need to account for a prefix at all. This would lead to drupal fields being stored as apigee attributes with the name field_myfieldname - but unless you are looking directly at the management API I don't see why that would be a problem. Whatever the final solution is, it is probably worth writing an update hook to copy existing data to the new attribute. |
We have added a dependency of Field UI module and rolled back the previous changes which will prevent such issues in future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I do not thing this approach is a good idea. In general the field_ui module is a UI module, its there help you configure fields on your content types (or teams). At almost every place I have worked in the last 12 years it has been disabled in production (and enabled in dev). This approach forces people to enable a UI module in production, which they wouldn't otherwise do, and it shouldn't be relevant to the functioning of the apigee module. Also. Lots of sites will loose their data when this is implemented. As stated in my previous comment, the problem is that the prefix changes. So anyone who currently does not have the field_ui module enabled will have all the field data stored as an apigee attribute field_myfieldname -> After they are forced to enabled the field_ui module as part of this change, this will no longer be the correct attribute name on apigee. |
/** | ||
* Install Field UI module to maintain dependency. | ||
*/ | ||
function apigee_edge_update_9003() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The update hook for Drupal 10+ should start with 10.
(Probably old update hooks should have been removed when 3.x branch started and Drupal core major version was bumped)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The update hook for Drupal 10+ should start with 10.
Thanks! Noted 👍
(Probably old update hooks should have been removed when 3.x branch started and Drupal core major version was bumped)
We will take care regarding this next time.
Because in Apigee management UI Custom attribute name size has limits, that is why we remove the prefix to increase the field name size. Solution (Including the changes in the PR): So that anyone who currently do not have the |
Closes #983
Added check for field_ui module enabled durning getting field prefix