Just noticed two discrepancies in __construct() between the annotations and the parameter definitions

the annotated param types for $config_factory and $query_factory are incorrect.

It is a good starter patch for anyone interested... otherwise I will get to this when I can...

I will answer any questions promptly, depending on timezone :)

CommentFileSizeAuthor
#16 2448915-16.patch1.31 KBmartin107
#3 interdiff-1-3.txt1.91 KBmartin107
#3 2448915-fieldStorageAddForm-constructor-annotation-3.patch1.68 KBmartin107
#1 2448915-fieldStorageAddForm-constructor-annotation-1.patch1.31 KBjoshi.rohit100
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 commented
Status: Active » Needs review
FileSize
2448915-fieldStorageAddForm-constructor-annotation-1.patch1.31 KB

Not sure whether QueryFactoy should be used or QueryFactoryInterface in constructor (usually we use interface) because if I see QueryFactoy class, it doesn't implement the given interface.

martin107’s picture

martin107 CreditAttribution: martin107 commented

Thanks joshi.rohit100 the patch looks good.

It addresses all my concerns. and make all the error flags my IDE places arround this code go away.

Looking at the QueryFactory and the QueryFactoryInterface

Yes I agree completely, at the moment we are incorrectly linking the Form to a specific implementation, we have lost flexibility, and should be linking it only to a interface. Here is the issue that introduced the interface #2019651: Add a QueryFactoryInterface for QueryFactory classes

Yes QueryFactory itself does not implement the interface, that has been carefully created to match it and I too think it is a bug. It certainly prevents the static analysis tools I am running on this project from doing the relevant double checks.

Its seems that this decoupling it not an accident!

https://www.drupal.org/node/2019651#comment-7537495

A most curious decision. Oh well who am I to blow against the wind....

In short I think you should just change the FieldStorageAddForm parameter definition.

martin107’s picture

martin107 CreditAttribution: martin107 commented
FileSize
2448915-fieldStorageAddForm-constructor-annotation-3.patch1.68 KB
interdiff-1-3.txt1.91 KB

Fixing the interface thing from #1

Status: Needs review » Needs work

The last submitted patch, 3: 2448915-fieldStorageAddForm-constructor-annotation-3.patch, failed testing.

martin107’s picture

martin107 CreditAttribution: martin107 commented

I am getting this same unexpected fail in another separate unrelated issue. So I think we might has one off those pesky test instability issues.

Argument 1 passed to Drupal\Core\Theme\ThemeManager::setActiveTheme() must be an instance of Drupal\Core\Theme\ActiveTheme, null given, called

Which needs to be confirmed and verified in another issue .. one moment.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Component: documentation » field system

This is not strictly documentation.

martin107 queued 3: 2448915-fieldStorageAddForm-constructor-annotation-3.patch for re-testing.

The last submitted patch, 3: 2448915-fieldStorageAddForm-constructor-annotation-3.patch, failed testing.

sarhugo’s picture

sarhugo CreditAttribution: sarhugo commented

If QueryFactory doesn't implement QueryFactoryInterface we should go with patch from #1. Other way the function signature will be wrong.

martin107’s picture

martin107 CreditAttribution: martin107 commented
Component: field system » documentation

Ok #2019651: Add a QueryFactoryInterface for QueryFactory classes

added the interface in such a way that it could not be used ... well that is an discussion for 8.1

I am happy for the patch from #1 to go forward an make this issue a documentation only patch again.

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 commented

So are we going with #1 ?

martin107’s picture

martin107 CreditAttribution: martin107 commented
Status: Needs work » Needs review

Yes I think that is a good idea.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

I'm confused about why in #1 the @var was changed from the interface back to the class, when the input to the constructor is the interface?

sarhugo’s picture

sarhugo CreditAttribution: sarhugo commented

Not for QueryFactory. The interface used is ConfigFactoryInterface. As mentioned at #9 the QueryFactory class doesn't implement QueryFactoryInterface.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Oh right. I was confusing QueryFactory and ConfigFactory. Sorry about that! Agreed with the changes.

Can someone upload the patch from #1 again so it is the last patch on the issue, and hide all the other files, to avoid confusion? Then it should be RTBC. I would do it but I don't deserve the patch credit. ;)

martin107’s picture

martin107 CreditAttribution: martin107 commented
Status: Needs work » Reviewed & tested by the community
FileSize
2448915-16.patch1.31 KB

posting patch from #1

martin107’s picture

martin107 CreditAttribution: martin107 commented
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Docs fixes are not frozen in beta. Committed 1ed3c6f and pushed to 8.0.x. Thanks!

  • alexpott committed 1ed3c6f on 8.0.x 
    Issue #2448915 by martin107, joshi.rohit100: FieldStorageAddForm...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.