Skip to content
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

Add field-based identifiers #13

Closed
wants to merge 7 commits into from
Closed

Add field-based identifiers #13

wants to merge 7 commits into from

Conversation

arourke
Copy link

@arourke arourke commented Dec 16, 2015

Allows for use of any field in the entity to be used as an identifier. Also updated readme. Identifier building was moved to its own function and various functions were added to accommodate these changes. See the updated readme and documentation within UploadBehavior.php for more information. Closes #11 by making all fields available.

Adds field-based identifiers and associated documentation. To
accommodate this, the building of identifiers has been moved to its own
function and various new functions have been added. Additionally, day of
month has been added as an identifier.
Adds to documentation
if unavailable, defaults to 'id' instead of nothing.
Added check which checks if array is sequential or associative. If it is
sequential, the data represents that from a hasMany or belongsToMany
association and is skipped accordingly.
Corrected typographical errors in order to pass Travis CI tests.
Further corrections to comply with Travis CI tests.
@Xety Xety added this to the v1.2 milestone Dec 16, 2015
@Xety
Copy link
Owner

Xety commented Dec 16, 2015

Great job, i really like it 👍

We also need tests for that before i merge.

@Xety
Copy link
Owner

Xety commented Dec 16, 2015

Just realized that i commented on your repo instead of mine... lol

Minor typographical corrections and code optimization as per Xety's
suggestions
@arourke
Copy link
Author

arourke commented Dec 17, 2015

Check the latest commit, most of your recommendations were adopted, but I'm a little confused on one of the changes you requested:

...if u can use only one space before and after the "$variable" name please...

When defining parameters for other functions, it appears that rather than having a single space before and after the "$variable" name, an effort was made to ensure that the parameter type, name, and description all had equal column widths. For instance:

    /**
     * Move the temporary source file to the destination file.
     *
     * @param \Cake\ORM\Entity $entity      The entity that is going to be saved.
     * @param bool|string      $source      The temporary source file to copy.
     * @param bool|string      $destination The destination file to copy.
     * @param bool|string      $field       The current field to process.
     * @param array            $options     The configuration options defined by the user.
     *
     * @return bool
     */
    protected function _moveFile(Entity $entity, $source = false, $destination = false, $field = false, array $options = [])

Are you suggesting that this should actually look like this?

    /**
     * @param \Cake\ORM\Entity $entity The entity that is going to be saved.
     * @param bool|string $source The temporary source file to copy.
     * @param bool|string $destination The destination file to copy.
     * @param bool|string $field The current field to process.
     * @param array $options The configuration options defined by the user.
     */

If this is the case, do you want me to change the notes for all of the functions in UploadBehavior.php to match the one space before/after convention?

Is this just a misinterpretation on my part?

I also realized after doing all this work that field identifiers don't work when adding a new entity. In fact, they cause the function to error out because the field does not appear in the entity, so when strtr($path, $identifiers) runs in _getUploadPath, the :field never gets replaced. This issue is exactly the same one discussed in issue #2. Perhaps the solution is to have the file save to a temporary location in beforeSave() callback and then build the path and move the file to the permanent location in the afterSave() callback?

As for the erroring out on add, I'm thinking i'll change the code so the whole path is sanitized one final time before being saved. This would mean if someone did use a field identifier, it would just save, verbatim, to the "field" location. To me, though, this is a bad workaround with many problems of its own.

As for test cases. I've never made any and am unfamiliar with the process. I was reading up on it and I think I could make the logic behind the test functions, but I'm unsure of what specific test functions need to be added. Also, would an additional fixture need to be created so that associated fields could be tested as well?

I look forward to your response.

@arourke arourke changed the title Feat add fields Add Field Based Identifiers Dec 17, 2015
@arourke arourke changed the title Add Field Based Identifiers Add Field-Based Identifiers Dec 17, 2015
@arourke arourke changed the title Add Field-Based Identifiers Add field-based identifiers Dec 17, 2015
@Xety
Copy link
Owner

Xety commented Dec 17, 2015

Are you suggesting that this should actually look like this?

Yes.

If this is the case, do you want me to change the notes for all of the functions in UploadBehavior.php to match the one space before/after convention?

It would be awesome please. 👍 If the parameter type, name, and description all had equal column widths, it's because i developed this plugin 1 year ago and it was like that 1 year ago. But Cake has changed his mind about their rules. (I actually need to update all my plugins because of that. 😝 )

This issue is exactly the same one discussed in issue #2.

Yes, that's the big problem of this plugin actually.
Bth with you, i developed this plugin for my need, my members can't upload an avatar when they register, but only in their Settings (So after that their accounts has been created), that's why i used the beforeFilter(). And i decided to release it on GitHub to help people starting with Cake3.

I see that my plugin is coming bigger each day, and i think i will start to rewrite it to use the correct callback methods. I'm working on a big project since 1 year, and i'm using this plugin in my app, and i need to modify the plugin to add a functionality to resize automatically an image using https://github.com/Intervention/image when uploading.

I think, the best option now, is to open an issue and discuss about the rewriting of this plugin :

  • The new structure of the plugin
  • What callback methods to use
  • New features (Like your feature, resize on fly, etc)
  • etc

And i think i will start to rewrite the plugin between christmas and the new year.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants